Prevent infinite loop rescanning the pack list on PackMismatchExceptionstable-5.9

We found, when analysing an incident where Gerrit's gc runner thread got
stuck, that we can end up in an infinite loop in
ObjectDirectory#openPackedObject which tries to rescan the pack
list and starts over trying to open a packed object in an unconfined
loop if it catches a PackMismatchException.

Here the relevant part of a thread dump we created while the gc runner
was stuck:

"WorkQueue-2[java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask@350812a3[Not
completed,
task = java.util.concurrent.Executors$RunnableAdapter@5425d7ee]]" #72
tid=0x00007f73cee1c800 nid=0x584
runnable  [0x00007f7392d57000]
   java.lang.Thread.State: RUNNABLE
	at org.eclipse.jgit.internal.storage.file.WindowCache.removeAll(WindowCache.java:716)
	at org.eclipse.jgit.internal.storage.file.WindowCache.purge(WindowCache.java:399)
	at org.eclipse.jgit.internal.storage.file.PackFile.close(PackFile.java:296)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectory.reuseMap(ObjectDirectory.java:973)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectory.scanPacksImpl(ObjectDirectory.java:904)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectory.scanPacks(ObjectDirectory.java:895)
	- locked <0x000000050a498f60> (a
java.util.concurrent.atomic.AtomicReference)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectory.searchPacksAgain(ObjectDirectory.java:794)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openPackedObject(ObjectDirectory.java:465)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openPackedFromSelfOrAlternate(ObjectDirectory.java:417)
	at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openObject(ObjectDirectory.java:408)
	at org.eclipse.jgit.internal.storage.file.WindowCursor.open(WindowCursor.java:132)
	at org.eclipse.jgit.lib.ObjectReader$1.open(ObjectReader.java:279)
	at org.eclipse.jgit.revwalk.RevWalk$2.next(RevWalk.java:1031)
	at org.eclipse.jgit.internal.storage.pack.PackWriter.findObjectsToPack(PackWriter.java:1911)
	at org.eclipse.jgit.internal.storage.pack.PackWriter.preparePack(PackWriter.java:960)
	at org.eclipse.jgit.internal.storage.pack.PackWriter.preparePack(PackWriter.java:876)
	at org.eclipse.jgit.internal.storage.file.GC.writePack(GC.java:1168)
	at org.eclipse.jgit.internal.storage.file.GC.repack(GC.java:852)
	at org.eclipse.jgit.internal.storage.file.GC.doGc(GC.java:269)
	at org.eclipse.jgit.internal.storage.file.GC.gc(GC.java:220)
	at org.eclipse.jgit.api.GarbageCollectCommand.call(GarbageCollectCommand.java:179)
	at com.google.gerrit.server.git.GarbageCollection.run(GarbageCollection.java:112)
	at com.google.gerrit.server.git.GarbageCollection.run(GarbageCollection.java:75)
	at com.google.gerrit.server.git.GarbageCollection.run(GarbageCollection.java:71)
	at com.google.gerrit.server.git.GarbageCollectionRunner.run(GarbageCollectionRunner.java:76)
	at com.google.gerrit.server.logging.LoggingContextAwareRunnable.run(LoggingContextAwareRunnable.java:103)
	at java.util.concurrent.Executors$RunnableAdapter.call(java.base@11.0.18/Executors.java:515)
	at java.util.concurrent.FutureTask.runAndReset(java.base@11.0.18/FutureTask.java:305)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(java.base@11.0.18/ScheduledThreadPoolExecutor.java:305)
	at com.google.gerrit.server.git.WorkQueue$Task.run(WorkQueue.java:612)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@11.0.18/ThreadPoolExecutor.java:1128)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@11.0.18/ThreadPoolExecutor.java:628)
	at java.lang.Thread.run(java.base@11.0.18/Thread.java:829)

The code in ObjectDirectory#openPackedObject [1] apparently assumes that
this is caused by a transient problem which it can resume from by
retrying. We use `core.trustFolderStat = false` on this server since it
uses NFS. The incident we had showed that we can enter into an infinite
loop here if there is a permanent mismatch between a pack file and its
corresponding pack index. I am not yet sure how this can happen.

Break the infinite loop by limiting the number of attempts rescanning
the pack list to 5 retries.  When we exceed this threshold set the type
of the PackMismatchException to permanent and rethrow it which breaks
the infinite loop.

Also apply the same limit in #getPackedObjectSize
and #selectObjectRepresentation where we use similar retry loops.

[1] https://git.eclipse.org/r/plugins/gitiles/jgit/jgit/+/011c26ff36b9e76c84fc2459e337f159c0f55a9a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java#465

Change-Id: I20fb63bcc1fdc3a03d39b963f06a90e6f0ba73dc

3 files changed, 76 insertions, 7 deletions

diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters
new file mode 100644
index 0000000000..7a3dc84085
--- /dev/null
+++ b/org.eclipse.jgit/.settings/.api_filters
@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<component id="org.eclipse.jgit" version="2">
+    <resource path="src/org/eclipse/jgit/errors/PackMismatchException.java" type="org.eclipse.jgit.errors.PackMismatchException">
+        <filter id="1142947843">
+            <message_arguments>
+                <message_argument value="5.9.1"/>
+                <message_argument value="isPermanent()"/>
+            </message_arguments>
+        </filter>
+        <filter id="1142947843">
+            <message_arguments>
+                <message_argument value="5.9.1"/>
+                <message_argument value="setPermanent(boolean)"/>
+            </message_arguments>
+        </filter>
+    </resource>
+</component>
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/errors/PackMismatchException.java b/org.eclipse.jgit/src/org/eclipse/jgit/errors/PackMismatchException.java
index ad5664ceb2..5f3f0c39fd 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/errors/PackMismatchException.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/errors/PackMismatchException.java
@@ -18,6 +18,8 @@ import java.io.IOException;
 public class PackMismatchException extends IOException {
 	private static final long serialVersionUID = 1L;
 
+	private boolean permanent;
+
 	/**
 	 * Construct a pack modification error.
 	 *
@@ -27,4 +29,31 @@ public class PackMismatchException extends IOException {
 	public PackMismatchException(String why) {
 		super(why);
 	}
+
+	/**
+	 * Set the type of the exception
+	 *
+	 * @param permanent
+	 *            whether the exception is considered permanent
+	 * @since 5.9.1
+	 */
+	public void setPermanent(boolean permanent) {
+		this.permanent = permanent;
+	}
+
+	/**
+	 * Check if this is a permanent problem
+	 *
+	 * @return if this is a permanent problem and repeatedly scanning the
+	 *         packlist couldn't fix it
+	 * @since 5.9.1
+	 */
+	public boolean isPermanent() {
+		return permanent;
+	}
+
+	@Override
+	public String toString() {
+		return super.toString() + ", permanent: " + permanent; //$NON-NLS-1$
+	}
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
index 9d1d457369..2131e5f670 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
@@ -89,6 +89,8 @@ public class ObjectDirectory extends FileObjectDatabase {
 	 *  handle exception is thrown */
 	final static int MAX_LOOSE_OBJECT_STALE_READ_ATTEMPTS = 5;
 
+	private static final int MAX_PACKLIST_RESCAN_ATTEMPTS = 5;
+
 	private final AlternateHandle handle = new AlternateHandle(this);
 
 	private final Config config;
@@ -413,7 +415,8 @@ public class ObjectDirectory extends FileObjectDatabase {
 	}
 
 	private ObjectLoader openPackedFromSelfOrAlternate(WindowCursor curs,
-			AnyObjectId objectId, Set<AlternateHandle.Id> skips) {
+			AnyObjectId objectId, Set<AlternateHandle.Id> skips)
+			throws PackMismatchException {
 		ObjectLoader ldr = openPackedObject(curs, objectId);
 		if (ldr != null) {
 			return ldr;
@@ -449,9 +452,11 @@ public class ObjectDirectory extends FileObjectDatabase {
 		return null;
 	}
 
-	ObjectLoader openPackedObject(WindowCursor curs, AnyObjectId objectId) {
+	ObjectLoader openPackedObject(WindowCursor curs, AnyObjectId objectId)
+			throws PackMismatchException {
 		PackList pList;
 		do {
+			int retries = 0;
 			SEARCH: for (;;) {
 				pList = packList.get();
 				for (PackFile p : pList.packs) {
@@ -462,8 +467,10 @@ public class ObjectDirectory extends FileObjectDatabase {
 							return ldr;
 					} catch (PackMismatchException e) {
 						// Pack was modified; refresh the entire pack list.
-						if (searchPacksAgain(pList))
+						if (searchPacksAgain(pList)) {
+							retries = checkRescanPackThreshold(retries, e);
 							continue SEARCH;
+						}
 					} catch (IOException e) {
 						handlePackError(e, p);
 					}
@@ -555,7 +562,8 @@ public class ObjectDirectory extends FileObjectDatabase {
 	}
 
 	private long getPackedSizeFromSelfOrAlternate(WindowCursor curs,
-			AnyObjectId id, Set<AlternateHandle.Id> skips) {
+			AnyObjectId id, Set<AlternateHandle.Id> skips)
+			throws PackMismatchException {
 		long len = getPackedObjectSize(curs, id);
 		if (0 <= len) {
 			return len;
@@ -590,9 +598,11 @@ public class ObjectDirectory extends FileObjectDatabase {
 		return -1;
 	}
 
-	private long getPackedObjectSize(WindowCursor curs, AnyObjectId id) {
+	private long getPackedObjectSize(WindowCursor curs, AnyObjectId id)
+			throws PackMismatchException {
 		PackList pList;
 		do {
+			int retries = 0;
 			SEARCH: for (;;) {
 				pList = packList.get();
 				for (PackFile p : pList.packs) {
@@ -603,8 +613,10 @@ public class ObjectDirectory extends FileObjectDatabase {
 							return len;
 					} catch (PackMismatchException e) {
 						// Pack was modified; refresh the entire pack list.
-						if (searchPacksAgain(pList))
+						if (searchPacksAgain(pList)) {
+							retries = checkRescanPackThreshold(retries, e);
 							continue SEARCH;
+						}
 					} catch (IOException e) {
 						handlePackError(e, p);
 					}
@@ -632,13 +644,14 @@ public class ObjectDirectory extends FileObjectDatabase {
 
 	@Override
 	void selectObjectRepresentation(PackWriter packer, ObjectToPack otp,
-																	WindowCursor curs) throws IOException {
+			WindowCursor curs) throws IOException {
 		selectObjectRepresentation(packer, otp, curs, null);
 	}
 
 	private void selectObjectRepresentation(PackWriter packer, ObjectToPack otp,
 			WindowCursor curs, Set<AlternateHandle.Id> skips) throws IOException {
 		PackList pList = packList.get();
+		int retries = 0;
 		SEARCH: for (;;) {
 			for (PackFile p : pList.packs) {
 				try {
@@ -649,6 +662,7 @@ public class ObjectDirectory extends FileObjectDatabase {
 				} catch (PackMismatchException e) {
 					// Pack was modified; refresh the entire pack list.
 					//
+					retries = checkRescanPackThreshold(retries, e);
 					pList = scanPacks(pList);
 					continue SEARCH;
 				} catch (IOException e) {
@@ -666,6 +680,15 @@ public class ObjectDirectory extends FileObjectDatabase {
 		}
 	}
 
+	private int checkRescanPackThreshold(int retries, PackMismatchException e)
+			throws PackMismatchException {
+		if (retries++ > MAX_PACKLIST_RESCAN_ATTEMPTS) {
+			e.setPermanent(true);
+			throw e;
+		}
+		return retries;
+	}
+
 	private void handlePackError(IOException e, PackFile p) {
 		String warnTmpl = null;
 		int transientErrorCount = 0;