Ensure repo is up-to-date when trying to build revisions

Summary:
The repository is in no way guaranteed to be up-to-date when the revision handler
is run, and since we have workers on multiple machines, the listener may have been
triggered by a commit we don't have yet.
This makes build_revision update the repo before trying to get a diff, which should
avoid this inconsistent scenario.
This pattern is also employed by the green build listener.
There is certainly a risk of races to update/clone, but from what I can tell
git/hg should be safe for multi-process use by default.
From what I can tell, this should resolve ~14k crashes in Sentry.
Some errors are still possible (races to clone being one), but they should
no worse than our current failures and much much rarer.

Test Plan: None

Reviewers: vishal, josiah

Reviewed By: josiah

Subscribers: josiah, changesbot, wwu

Differential Revision: https://tails.corp.dropbox.com/D97481

changes/listeners/build_revision.py

@@ -8,6 +8,7 @@
 from changes.models import ProjectStatus, Project, ProjectOptionsHelper, Revision
 from changes.utils.diff_parser import DiffParser
 from changes.utils.whitelist import in_project_files_whitelist
+from changes.vcs.base import UnknownRevision
 
 
 def revision_created_handler(revision_sha, repository_id, **kwargs):
@@ -48,8 +49,20 @@ def get_changed_files(self):
         vcs = self.repository.get_vcs()
         if not vcs:
             raise NotImplementedError
+        # Make sure the repo exists on disk.
+        if not vcs.exists():
+            vcs.clone()
+
+        diff = None
+        try:
+            diff = vcs.export(self.revision.sha)
+        except UnknownRevision:
+            # Maybe the repo is stale; update.
+            vcs.update()
+            # If it doesn't work this time, we have
+            # a problem. Let the exception escape.
+            diff = vcs.export(self.revision.sha)
 
-        diff = vcs.export(self.revision.sha)
         diff_parser = DiffParser(diff)
         return diff_parser.get_changed_files()
 

changes/vcs/base.py

@@ -26,6 +26,8 @@ def __str__(self):
 
 
 class UnknownRevision(CommandError):
+    """Indicates that an operation was attempted on a
+    revision that doesn't appear to exist."""
     pass
 
 
@@ -123,6 +125,14 @@ def log(self, parent=None, branch=None, author=None, offset=0, limit=100):
         raise NotImplementedError
 
     def export(self, id):
+        """Get the textual diff for a revision.
+        Args:
+            id (str): The id of the revision.
+        Returns:
+            A string with the text of the diff for the revision.
+        Raises:
+            UnknownRevision: If the revision wasn't found.
+        """
         raise NotImplementedError
 
     def get_default_revision(self):

changes/vcs/git.py

@@ -201,6 +201,14 @@ def log(self, parent=None, branch=None, author=None, offset=0, limit=100):
             )
 
     def export(self, id):
+        """Get the textual diff for a revision.
+        Args:
+            id (str): The id of the revision.
+        Returns:
+            A string with the text of the diff for the revision.
+        Raises:
+            UnknownRevision: If the revision wasn't found.
+        """
         cmd = ['diff', '%s^..%s' % (id, id)]
         result = self.run(cmd)
         return result

changes/vcs/hg.py

@@ -141,6 +141,14 @@ def log(self, parent=None, branch=None, author=None, offset=0, limit=100):
             )
 
     def export(self, id):
+        """Get the textual diff for a revision.
+        Args:
+            id (str): The id of the revision.
+        Returns:
+            A string with the text of the diff for the revision.
+        Raises:
+            UnknownRevision: If the revision wasn't found.
+        """
         cmd = ['diff', '-g', '-c %s' % (id,)]
         result = self.run(cmd)
         return result

tests/changes/listeners/test_build_revision.py

@@ -1,12 +1,14 @@
 from __future__ import absolute_import, print_function
 
 from mock import Mock, patch
+from uuid import uuid4
 
 from changes.config import db
-from changes.listeners.build_revision import revision_created_handler
+from changes.listeners.build_revision import revision_created_handler, CommitTrigger
 from changes.models import Build, ProjectOption
 from changes.testutils.cases import TestCase
 from changes.testutils.fixtures import SAMPLE_DIFF
+from changes.vcs.base import UnknownRevision
 
 
 class RevisionCreatedHandlerTestCase(TestCase):
@@ -70,3 +72,55 @@ def test_file_whitelist(self, mock_identify_revision, mock_get_vcs):
         mock_identify_revision.assert_called_once_with(repo, revision.sha)
 
         assert Build.query.first()
+
+    def test_get_changed_files_updates_vcs(self):
+        repo = self.create_repo()
+        sha = uuid4().hex
+        revision = self.create_revision(repository=repo, sha=sha)
+
+        # No updated needed.
+        with patch.object(repo, 'get_vcs') as get_vcs:
+            mock_vcs = Mock()
+            mock_vcs.exists.return_value = True
+            mock_vcs.export.return_value = SAMPLE_DIFF
+            get_vcs.return_value = mock_vcs
+            ct = CommitTrigger(revision)
+            ct.get_changed_files()
+            self.assertEqual(list(mock_vcs.method_calls), [
+                ('exists', (), {}),
+                ('export', (sha,), {}),
+                ])
+
+        # Successful update
+        with patch.object(repo, 'get_vcs') as get_vcs:
+            mock_vcs = Mock()
+            mock_vcs.exists.return_value = True
+            # Raise first time, work second time.
+            mock_vcs.export.side_effect = (UnknownRevision("", 1), SAMPLE_DIFF)
+            get_vcs.return_value = mock_vcs
+            ct = CommitTrigger(revision)
+            ct.get_changed_files()
+            self.assertEqual(list(mock_vcs.method_calls), [
+                ('exists', (), {}),
+                ('export', (sha,), {}),
+                ('update', (), {}),
+                ('export', (sha,), {}),
+                ])
+
+        # Unsuccessful update
+        with patch.object(repo, 'get_vcs') as get_vcs:
+            mock_vcs = Mock()
+            mock_vcs.exists.return_value = True
+            # Revision is always unknown.
+            mock_vcs.export.side_effect = UnknownRevision("", 1)
+            get_vcs.return_value = mock_vcs
+
+            ct = CommitTrigger(revision)
+            with self.assertRaises(UnknownRevision):
+                ct.get_changed_files()
+            self.assertEqual(list(mock_vcs.method_calls), [
+                ('exists', (), {}),
+                ('export', (sha,), {}),
+                ('update', (), {}),
+                ('export', (sha,), {}),
+                ])