Return cached Github change on concurrent update

When a pull-request is updated concurrently on a scheduler we'll wait
until the first thread has updated the change. The problem is if we
needed to create a new PR object. In this case we'd return a Github
change that wasn't updated and also doesn't have a cache key set.

ERROR zuul.Scheduler: Exception processing pipeline check in tenant foobar
Traceback (most recent call last):
  File "/opt/zuul/lib/python3.10/site-packages/zuul/scheduler.py", line 2149, in process_pipelines
    refreshed = self._process_pipeline(
  File "/opt/zuul/lib/python3.10/site-packages/zuul/scheduler.py", line 2241, in _process_pipeline
    self.process_pipeline_trigger_queue(tenant, pipeline)
  File "/opt/zuul/lib/python3.10/site-packages/zuul/scheduler.py", line 2447, in process_pipeline_trigger_queue
    self._process_trigger_event(tenant, pipeline, event)
  File "/opt/zuul/lib/python3.10/site-packages/zuul/scheduler.py", line 2480, in _process_trigger_event
    pipeline.manager.addChange(change, event)
  File "/opt/zuul/lib/python3.10/site-packages/zuul/manager/__init__.py", line 534, in addChange
    self.updateCommitDependencies(change, None, event)
  File "/opt/zuul/lib/python3.10/site-packages/zuul/manager/__init__.py", line 868, in updateCommitDependencies
    new_commit_needs_changes = [d.cache_key for d in dependencies]
  File "/opt/zuul/lib/python3.10/site-packages/zuul/manager/__init__.py", line 868, in <listcomp>
    new_commit_needs_changes = [d.cache_key for d in dependencies]
  File "/opt/zuul/lib/python3.10/site-packages/zuul/model.py", line 5946, in cache_key
    return self.cache_stat.key.reference
AttributeError: 'NoneType' object has no attribute 'key'

Change-Id: I2f3012060c486d593ad857e046334c3d9bef0ed5

2 files changed, 43 insertions, 1 deletions

diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py
index e3706440b..fb46aa7d1 100644
--- a/tests/unit/test_github_driver.py
+++ b/tests/unit/test_github_driver.py
@@ -18,8 +18,10 @@ import re
 from testtools.matchers import MatchesRegex, Not, StartsWith
 import urllib
 import socket
+import threading
 import time
 import textwrap
+from concurrent.futures import ThreadPoolExecutor
 from unittest import mock, skip
 
 import git
@@ -32,10 +34,11 @@ from zuul.zk.layout import LayoutState
 from zuul.lib import strings
 from zuul.merger.merger import Repo
 from zuul.model import MergeRequest, EnqueueEvent, DequeueEvent
+from zuul.zk.change_cache import ChangeKey
 
 from tests.base import (AnsibleZuulTestCase, BaseTestCase,
                         ZuulGithubAppTestCase, ZuulTestCase,
-                        simple_layout, random_sha1)
+                        simple_layout, random_sha1, iterate_timeout)
 from tests.base import ZuulWebFixture
 
 EMPTY_LAYOUT_STATE = LayoutState("", "", 0, None, {}, -1)
@@ -1484,6 +1487,44 @@ class TestGithubDriver(ZuulTestCase):
             "rebase not supported",
             str(loading_errors[0].error))
 
+    @simple_layout("layouts/basic-github.yaml", driver="github")
+    def test_concurrent_get_change(self):
+        """
+        Test that getting a change concurrently returns the same
+        object from the cache.
+        """
+        conn = self.scheds.first.sched.connections.connections["github"]
+
+        # Create a new change object and remove it from the cache so
+        # the concurrent call will try to create a new change object.
+        A = self.fake_github.openFakePullRequest("org/project", "master", "A")
+        change_key = ChangeKey(conn.connection_name, "org/project",
+                               "PullRequest", str(A.number), str(A.head_sha))
+        change = conn.getChange(change_key, refresh=True)
+        conn._change_cache.delete(change_key)
+
+        # Acquire the update lock so the concurrent get task needs to
+        # wait for the lock to be release.
+        lock = conn._change_update_lock.setdefault(change_key,
+                                                   threading.Lock())
+        lock.acquire()
+        try:
+            executor = ThreadPoolExecutor(max_workers=1)
+            task = executor.submit(conn.getChange, change_key, refresh=True)
+            for _ in iterate_timeout(5, "task to be running"):
+                if task.running():
+                    break
+            # Add the change back so the waiting task can get the
+            # change from the cache.
+            conn._change_cache.set(change_key, change)
+        finally:
+            lock.release()
+            executor.shutdown()
+
+        other_change = task.result()
+        self.assertIsNotNone(other_change.cache_stat)
+        self.assertIs(change, other_change)
+
 
 class TestMultiGithubDriver(ZuulTestCase):
     config_file = 'zuul-multi-github.conf'
diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py
index 631d98998..182c83bae 100644
--- a/zuul/driver/github/githubconnection.py
+++ b/zuul/driver/github/githubconnection.py
@@ -1461,6 +1461,7 @@ class GithubConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
             log.debug("Change %s is currently being updated, "
                       "waiting for it to finish", change)
             with lock:
+                change = self._change_cache.get(change_key)
                 log.debug('Finished updating change %s', change)
         return change