From 60f846aa7a5abfd7e7ffcc6918bf138c4f112fc9 Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jim@acmegating.com>
Date: Wed, 18 Aug 2021 16:37:18 -0700
Subject: [PATCH] Fix error with config cache some more

The previous commit to fix errors with the commit cache overlooked
the fact that the merger returns explicit dictionary entries for
all filenames specifically requested.  So to determine that a file
under zuul.d/ does not exist, it is sufficient to check if the
path is in the files dict returned by the merger.  But it is
insufficient to perform the same check for zuul.yaml since it
is a specific file requested, not a dir.

The merger does return None as the content of those entries if
the file does not exist.  To accomodate this, test for a None value
in addition to membership.  Also, add a test to cover this case.

Additionally, it was noted that the new debug log entry was reporting
the wrong project name.  This did not affect the cache cleanup, but
the same value is used to lock the cache.  This is because both lines
were referencing the leftover "project" variable from the loop above.
Both lines are updated to use the correct variable, and the project
variable is removed from scope to avoid future confusion.

Change-Id: Ie723d13a3ec5156ff19fbb695c5314e46aee2e02
---
 tests/unit/test_v3.py | 49 +++++++++++++++++++++++++++++++++++++++++++
 zuul/configloader.py  |  9 +++++---
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py
index 2f3df70848..60e791a977 100644
--- a/tests/unit/test_v3.py
+++ b/tests/unit/test_v3.py
@@ -2440,6 +2440,55 @@ class TestInRepoConfig(ZuulTestCase):
         self.assertIn('Debug information:',
                       A.messages[0], "A should have debug info")
 
+    def test_file_move(self):
+        # Tests that a zuul config file can be renamed
+        in_repo_conf = textwrap.dedent(
+            """
+            - job:
+                name: project-test2
+                parent: project-test1
+
+            - project:
+                check:
+                  jobs:
+                    - project-test2
+            """)
+        file_dict = {'.zuul.yaml': None,
+                     '.zuul.d/newfile.yaml': in_repo_conf}
+        A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A',
+                                           files=file_dict)
+        A.setMerged()
+        self.fake_gerrit.addEvent(A.getChangeMergedEvent())
+        self.waitUntilSettled()
+
+        B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([
+            dict(name='project-test2', result='SUCCESS', changes='2,1'),
+        ], ordered=True)
+
+        self.scheds[0].sched.stop()
+        self.scheds[0].sched.join()
+        del self.scheds[0]
+        self.log.debug("Restarting scheduler")
+        self.createScheduler()
+        self.scheds[0].start(self.validate_tenants)
+
+        self.waitUntilSettled()
+
+        # The fake gerrit was lost with the scheduler shutdown;
+        # restore the state we care about:
+        self.fake_gerrit.change_number = 2
+
+        C = self.fake_gerrit.addFakeChange('org/project1', 'master', 'C')
+        self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([
+            dict(name='project-test2', result='SUCCESS', changes='2,1'),
+            dict(name='project-test2', result='SUCCESS', changes='3,1'),
+        ], ordered=True)
+
 
 class TestInRepoConfigDir(ZuulTestCase):
     # Like TestInRepoConfig, but the fixture test files are in zuul.d
diff --git a/zuul/configloader.py b/zuul/configloader.py
index 4f246ec60e..3d9553901f 100644
--- a/zuul/configloader.py
+++ b/zuul/configloader.py
@@ -1818,6 +1818,7 @@ class TenantParser(object):
                 job.ltime = ltime
                 job.source_context = source_context
                 jobs.append(job)
+        del project
 
         for job in jobs:
             self.log.debug("Waiting for cat job %s" % (job,))
@@ -1839,7 +1840,8 @@ class TenantParser(object):
             files_cache = self.unparsed_config_cache.getFilesCache(
                 job.source_context.project.canonical_name,
                 job.source_context.branch)
-            with self.unparsed_config_cache.writeLock(project.canonical_name):
+            with self.unparsed_config_cache.writeLock(
+                    job.source_context.project.canonical_name):
                 for fn, content in job.files.items():
                     # Cache file in Zookeeper
                     if content is not None:
@@ -1854,11 +1856,12 @@ class TenantParser(object):
                 for fn in files_cache.keys():
                     if ((fn in ok_files) or
                         any([fn.startswith(d + '/') for d in ok_dirs])):
-                        if fn not in job.files:
+                        if job.files.get(fn) is None:
                             self.log.debug(
                                 "Removing file from cache for project "
                                 "%s @%s: %s",
-                                project.canonical_name, branch, fn)
+                                job.source_context.project.canonical_name,
+                                branch, fn)
                             del files_cache[fn]
                 files_cache.setValidFor(job.extra_config_files,
                                         job.extra_config_dirs,