Don't share state in build task

readthedocs/projects/tasks.py

@@ -161,14 +161,34 @@ def _log(self, msg):
                          msg=msg))
 
 
-class SyncRepositoryTask(SyncRepositoryMixin, Task):
+# TODO SyncRepositoryTask should be refactored into a standard celery task,
+# there is no more need to have this be a separate class
+class SyncRepositoryTask(Task):
 
-    """Entry point to synchronize the VCS documentation."""
+    """Celery task to trigger VCS version sync."""
 
     max_retries = 5
     default_retry_delay = (7 * 60)
     name = __name__ + '.sync_repository'
 
+    def run(self, *args, **kwargs):
+        step = SyncRepositoryTaskStep()
+        return step.run(*args, **kwargs)
+
+
+class SyncRepositoryTaskStep(SyncRepositoryMixin):
+
+    """Entry point to synchronize the VCS documentation.
+
+    .. note::
+
+        This is implemented as a separate class to isolate each run of the
+        underlying task. Previously, we were using a custom ``celery.Task`` for
+        this, but this class is only instantiated once -- on startup. The effect
+        was that this instance shared state between workers.
+
+    """
+
     def run(self, version_pk):  # pylint: disable=arguments-differ
         """
         Run the VCS synchronization.
@@ -194,21 +214,37 @@ def run(self, version_pk):  # pylint: disable=arguments-differ
         return False
 
 
-class UpdateDocsTask(SyncRepositoryMixin, Task):
+# TODO UpdateDocsTask should be refactored into a standard celery task,
+# there is no more need to have this be a separate class
+class UpdateDocsTask(Task):
+
+    max_retries = 5
+    default_retry_delay = (7 * 60)
+    name = __name__ + '.update_docs'
+
+    def run(self, *args, **kwargs):
+        step = UpdateDocsTaskStep()
+        return step.run(*args, **kwargs)
+
+
+class UpdateDocsTaskStep(SyncRepositoryMixin):
 
     """
     The main entry point for updating documentation.
 
     It handles all of the logic around whether a project is imported, we created
     it or a webhook is received. Then it will sync the repository and build the
     html docs if needed.
-    """
 
-    max_retries = 5
-    default_retry_delay = (7 * 60)
-    name = __name__ + '.update_docs'
+    .. note::
+
+        This is implemented as a separate class to isolate each run of the
+        underlying task. Previously, we were using a custom ``celery.Task`` for
+        this, but this class is only instantiated once -- on startup. The effect
+        was that this instance shared state between workers.
+
+    """
 
-    # TODO: the argument from the __init__ are used only in tests
     def __init__(self, build_env=None, python_env=None, config=None,
                  force=False, search=True, localmedia=True,
                  build=None, project=None, version=None):

readthedocs/rtd_tests/tests/test_builds.py

@@ -12,7 +12,7 @@
 from readthedocs.doc_builder.environments import LocalBuildEnvironment
 from readthedocs.doc_builder.python_environments import Virtualenv
 from readthedocs.doc_builder.loader import get_builder_class
-from readthedocs.projects.tasks import UpdateDocsTask
+from readthedocs.projects.tasks import UpdateDocsTaskStep
 from readthedocs.rtd_tests.tests.test_config_wrapper import create_load
 
 from ..mocks.environment import EnvironmentMockGroup
@@ -44,7 +44,7 @@ def test_build(self):
         build_env = LocalBuildEnvironment(project=project, version=version, build={})
         python_env = Virtualenv(version=version, build_env=build_env)
         config = ConfigWrapper(version=version, yaml_config=create_load()()[0])
-        task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env,
+        task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env,
                               version=version, search=False, localmedia=False, config=config)
         task.build_docs()
 
@@ -68,7 +68,7 @@ def test_build_respects_pdf_flag(self):
         build_env = LocalBuildEnvironment(project=project, version=version, build={})
         python_env = Virtualenv(version=version, build_env=build_env)
         config = ConfigWrapper(version=version, yaml_config=create_load()()[0])
-        task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env,
+        task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env,
                               version=version, search=False, localmedia=False, config=config)
 
         task.build_docs()
@@ -93,7 +93,7 @@ def test_build_respects_epub_flag(self):
         build_env = LocalBuildEnvironment(project=project, version=version, build={})
         python_env = Virtualenv(version=version, build_env=build_env)
         config = ConfigWrapper(version=version, yaml_config=create_load()()[0])
-        task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env,
+        task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env,
                               version=version, search=False, localmedia=False, config=config)
         task.build_docs()
 
@@ -119,7 +119,7 @@ def test_build_respects_yaml(self):
         config = ConfigWrapper(version=version, yaml_config=create_load({
             'formats': ['epub']
         })()[0])
-        task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env,
+        task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env,
                               version=version, search=False, localmedia=False, config=config)
         task.build_docs()
 
@@ -174,7 +174,7 @@ def test_build_pdf_latex_failures(self):
         build_env = LocalBuildEnvironment(project=project, version=version, build={})
         python_env = Virtualenv(version=version, build_env=build_env)
         config = ConfigWrapper(version=version, yaml_config=create_load()()[0])
-        task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env,
+        task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env,
                               version=version, search=False, localmedia=False, config=config)
 
         # Mock out the separate calls to Popen using an iterable side_effect
@@ -216,7 +216,7 @@ def test_build_pdf_latex_not_failure(self):
         build_env = LocalBuildEnvironment(project=project, version=version, build={})
         python_env = Virtualenv(version=version, build_env=build_env)
         config = ConfigWrapper(version=version, yaml_config=create_load()()[0])
-        task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env,
+        task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env,
                               version=version, search=False, localmedia=False, config=config)
 
         # Mock out the separate calls to Popen using an iterable side_effect

readthedocs/rtd_tests/tests/test_celery.py

@@ -64,9 +64,9 @@ def test_clear_artifacts(self):
         self.assertTrue(result.successful())
         self.assertFalse(exists(directory))
 
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock)
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock)
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock)
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock)
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs', new=MagicMock)
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_vcs', new=MagicMock)
     def test_update_docs(self):
         build = get(Build, project=self.project,
                     version=self.project.versions.first())
@@ -79,10 +79,10 @@ def test_update_docs(self):
                 intersphinx=False)
         self.assertTrue(result.successful())
 
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock)
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock)
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock)
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs', new=MagicMock)
     @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock)
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs')
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_vcs')
     def test_update_docs_unexpected_setup_exception(self, mock_setup_vcs):
         exc = Exception()
         mock_setup_vcs.side_effect = exc
@@ -97,10 +97,10 @@ def test_update_docs_unexpected_setup_exception(self, mock_setup_vcs):
                 intersphinx=False)
         self.assertTrue(result.successful())
 
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock)
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock)
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock)
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_vcs', new=MagicMock)
     @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock)
-    @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs')
+    @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs')
     def test_update_docs_unexpected_build_exception(self, mock_build_docs):
         exc = Exception()
         mock_build_docs.side_effect = exc