Merge "Cleanup leaked git index.lock files on checkout"

tests/unit/test_merger_repo.py

@@ -960,10 +960,6 @@ class TestMerger(ZuulTestCase):
         self.waitUntilSettled()
         self.assertEqual(A.data['status'], 'MERGED')
 
-        # Stop the merger so we can modify the git repo
-        self.merge_server.stop()
-        self.merge_server.join()
-
         # Add an index.lock file
         fpath = os.path.join(self.merger_src_root, 'review.example.com',
                              'org', 'org%2Fproject1', '.git', 'index.lock')
@@ -971,9 +967,6 @@ class TestMerger(ZuulTestCase):
             pass
         self.assertTrue(os.path.exists(fpath))
 
-        # Start a new merger and check that we can still merge things
-        self._startMerger()
-
         # This will fail if git can't modify the repo due to a stale lock file.
         B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
         B.addApproval('Code-Review', 2)

zuul/merger/merger.py

@@ -546,16 +546,29 @@ class Repo(object):
             log.debug("Repo is already at %s" % ref)
         else:
             log.debug("Checking out %s" % ref)
-            # Perform a hard reset to the correct ref before checking out so
-            # that we clean up anything that might be left over from a merge
-            # while still only preparing the working copy once.
-            repo.head.reference = ref
-            repo.head.reset(working_tree=True)
-            repo.git.clean('-x', '-f', '-d')
-            repo.git.checkout(ref)
-
+            try:
+                self._checkout(repo, ref)
+            except Exception:
+                lock_path = f"{self.local_path}/.git/index.lock"
+                if os.path.isfile(lock_path):
+                    log.warning("Deleting stale index.lock file: %s",
+                                lock_path)
+                    os.unlink(lock_path)
+                    # Retry the checkout
+                    self._checkout(repo, ref)
+                else:
+                    raise
         return repo.head.commit
 
+    def _checkout(self, repo, ref):
+        # Perform a hard reset to the correct ref before checking out so
+        # that we clean up anything that might be left over from a merge
+        # while still only preparing the working copy once.
+        repo.head.reference = ref
+        repo.head.reset(working_tree=True)
+        repo.git.clean('-x', '-f', '-d')
+        repo.git.checkout(ref)
+
     def cherryPick(self, ref, zuul_event_id=None):
         log = get_annotated_logger(self.log, zuul_event_id)
         repo = self.createRepoObject(zuul_event_id)

zuul/merger/server.py

@@ -15,7 +15,6 @@
 
 import json
 import logging
-import os
 import socket
 import sys
 import threading
@@ -156,24 +155,6 @@ class BaseMergeServer(metaclass=ABCMeta):
 
     def start(self):
         self.log.debug('Starting merger')
-        self.log.debug('Cleaning any stale git index.lock files')
-        for (dirpath, dirnames, filenames) in os.walk(self.merge_root):
-            if '.git' in dirnames:
-                # Only recurse into .git dirs
-                dirnames.clear()
-                dirnames.append('.git')
-            elif dirpath.endswith('/.git'):
-                # Recurse no further
-                dirnames.clear()
-                if 'index.lock' in filenames:
-                    fp = os.path.join(dirpath, 'index.lock')
-                    try:
-                        os.unlink(fp)
-                        self.log.debug('Removed stale git lock: %s' % fp)
-                    except Exception:
-                        self.log.exception(
-                            'Unable to remove stale git lock: '
-                            '%s this may result in failed merges' % fp)
         self._merger_running = True
         self.merger_thread.start()