[pinpoint] Fix race condition in building + isolate finding.

dashboard/dashboard/pinpoint/models/quest/find_isolate.py

Index: dashboard/dashboard/pinpoint/models/quest/find_isolate.py
diff --git a/dashboard/dashboard/pinpoint/models/quest/find_isolate.py b/dashboard/dashboard/pinpoint/models/quest/find_isolate.py
index ef1a0e4254500bbf0c42454353a43fc5d77c42d2..ca382e768a922b6c21d9f8d4e355eba71fa10231 100644
--- a/dashboard/dashboard/pinpoint/models/quest/find_isolate.py
+++ b/dashboard/dashboard/pinpoint/models/quest/find_isolate.py
@@ -49,37 +49,56 @@ class _FindIsolateExecution(execution.Execution):
     }
 
   def _Poll(self):
-    # Look for the .isolate in our cache.
+    if self._CheckIsolateCache():
+      return
+
+    if self._build:
+      self._CheckBuildStatus()
+      return
+
+    self._RequestBuild()
+
+  def _CheckIsolateCache(self):
+    """Checks the isolate cache to see if a build is already available.
+
+    Returns:
+      True iff the isolate was found and the execution is completed.

[perezju, 2017/09/13 13:46:38]

nit: Maybe this should be called "_CheckCompleted"? Which returns: "True if the
isolate was found, meaning the execution is completed."

[dtu, 2017/09/13 15:16:05]

Done.

+    """
     try:
       isolate_hash = isolate.Get(self._builder_name, self._change, self._target)
     except KeyError:
-      isolate_hash = None
+      return False
+    self._Complete(result_arguments={'isolate_hash': isolate_hash})
+    return True
 
-    if isolate_hash:
-      self._Complete(
-          result_arguments={'isolate_hash': isolate_hash})
-      return
+  def _CheckBuildStatus(self):
+    """Checks on the status of a previously requested build.
 
-    # Check the status of a previously requested build.
-    if self._build:
-      status = buildbucket_service.GetJobStatus(self._build)
+    Raises:
+      BuildError: The build failed, was canceled, or didn't produce an isolate.
+    """
+    status = buildbucket_service.GetJobStatus(self._build)
 
-      if status['build']['status'] != 'COMPLETED':
+    if status['build']['status'] != 'COMPLETED':
+      return
+
+    if status['build']['result'] == 'FAILURE':
+      raise BuildError('Build failed: ' + status['build']['failure_reason'])
+    elif status['build']['result'] == 'CANCELED':
+      raise BuildError('Build was canceled: ' +
+                       status['build']['cancelation_reason'])
+    else:
+      if self._CheckIsolateCache():
         return
+      raise BuildError('Buildbucket says the build completed successfully, '
+                       "but Pinpoint can't find the isolate hash.")
 
-      if status['build']['result'] == 'FAILURE':
-        raise BuildError('Build failed: ' + status['build']['failure_reason'])
-      elif status['build']['result'] == 'CANCELED':
-        raise BuildError('Build was canceled: ' +
-                         status['build']['cancelation_reason'])
-      else:
-        # It's possible for there to be a race condition if the builder uploads
-        # the isolate and completes the build between the above isolate lookup
-        # and buildbucket lookup, but right now, it takes builds a few minutes
-        # to package the build, so that doesn't happen.
-        raise BuildError('Buildbucket says the build completed successfully, '
-                         "but Pinpoint can't find the isolate hash.")
+  def _RequestBuild(self):
+    """Requests a build.
 
+    If a previous Execution already requested a build for this Change, returns
+    that build instead of requesting a new one.
+    """
     if self._change in self._previous_builds:
       # If another Execution already requested a build, reuse that one.
       self._build = self._previous_builds[self._change]