Restore preserved packs during missing object seeks

Provide a recovery path for objects being referenced during the pack
pruning race. Due to the pack pruning race, it is possible for objects
to become referenced after a pack has been deemed safe to prune, but
before it actually gets pruned. If this happened previously, the newly
referenced objects would be missing and potentially result in a
corrupted ref.

Add the ability to recover from this situation when an object is missing
but happens to still be available in a pack in the "preserved"
directory. This is likely only useful when used in conjunction with the
--preserve-old-packs GC option, which prunes packs by hard-linking to
the preserved directory. If an object is missing and found in a pack in
the preserved directory, immediately recover that pack and its
associated files (idx, bitmaps...) by moving them back to the original
pack directory, and then retry the operation that would have failed due
to the missing object. This retry can now succeed and the repository
may avoid corruption. This approach should drastically reduce the
chance of a corrupt repository during pack pruning at very little extra
cost. This extra cost should only be incurred when objects are missing
and a failure would normally occur.

Change-Id: I2a704e3276b88cc892159d9bfe2455c6eec64252
Signed-off-by: Martin Fick <quic_mfick@quicinc.com>
Signed-off-by: Nasser Grainawi <quic_nasserg@quicinc.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java

@@ -23,6 +23,7 @@ import java.util.List;
 
 import org.eclipse.jgit.junit.TestRepository.BranchBuilder;
 import org.eclipse.jgit.lib.ConfigConstants;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
@@ -314,6 +315,56 @@ public class GcBasicPackingTest extends GcTestCase {
 		assertTrue(preservedPackFile.exists());
 	}
 
+	@Test
+	public void testPruneAndRestoreOldPacks() throws Exception {
+		String tempRef = "refs/heads/soon-to-be-unreferenced";
+		BranchBuilder bb = tr.branch(tempRef);
+		bb.commit().add("A", "A").add("B", "B").create();
+
+		// Verify setup conditions
+		stats = gc.getStatistics();
+		assertEquals(4, stats.numberOfLooseObjects);
+		assertEquals(0, stats.numberOfPackedObjects);
+
+		// Force all referenced objects into packs (to avoid having loose objects)
+		configureGc(gc, false);
+		gc.setExpireAgeMillis(0);
+		gc.setPackExpireAgeMillis(0);
+		gc.gc();
+		stats = gc.getStatistics();
+		assertEquals(0, stats.numberOfLooseObjects);
+		assertEquals(4, stats.numberOfPackedObjects);
+		assertEquals(1, stats.numberOfPackFiles);
+
+		// Delete the temp ref, orphaning its commit
+		RefUpdate update = tr.getRepository().getRefDatabase().newUpdate(tempRef, false);
+		update.setForceUpdate(true);
+		ObjectId objectId = update.getOldObjectId(); // remember it so we can restore it!
+		RefUpdate.Result result = update.delete();
+		assertEquals(RefUpdate.Result.FORCED, result);
+
+		fsTick();
+
+		// Repack with only orphaned commit, so packfile will be pruned
+		configureGc(gc, false).setPreserveOldPacks(true);
+		gc.gc();
+		stats = gc.getStatistics();
+		assertEquals(0, stats.numberOfLooseObjects);
+		assertEquals(0, stats.numberOfPackedObjects);
+		assertEquals(0, stats.numberOfPackFiles);
+
+		// Restore the temp ref to the deleted commit, should restore old-packs!
+		update = tr.getRepository().getRefDatabase().newUpdate(tempRef, false);
+		update.setNewObjectId(objectId);
+		update.setExpectedOldObjectId(null);
+		result = update.update();
+		assertEquals(RefUpdate.Result.NEW, result);
+
+		stats = gc.getStatistics();
+		assertEquals(4, stats.numberOfPackedObjects);
+		assertEquals(1, stats.numberOfPackFiles);
+	}
+
 	private PackConfig configureGc(GC myGc, boolean aggressive) {
 		PackConfig pconfig = new PackConfig(repo);
 		if (aggressive) {

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java

@@ -13,6 +13,7 @@ package org.eclipse.jgit.internal.storage.file;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK;
 import static org.eclipse.jgit.internal.storage.pack.PackExt.BITMAP_INDEX;
+import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX;
 
 import java.io.BufferedReader;
 import java.io.File;
@@ -31,6 +32,7 @@ import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.pack.ObjectToPack;
+import org.eclipse.jgit.internal.storage.pack.PackExt;
 import org.eclipse.jgit.internal.storage.pack.PackWriter;
 import org.eclipse.jgit.lib.AbbreviatedObjectId;
 import org.eclipse.jgit.lib.AnyObjectId;
@@ -78,7 +80,7 @@ public class ObjectDirectory extends FileObjectDatabase {
 
 	private final PackDirectory packed;
 
-	private final File preservedDirectory;
+	private final PackDirectory preserved;
 
 	private final File alternatesFile;
 
@@ -116,10 +118,11 @@ public class ObjectDirectory extends FileObjectDatabase {
 		objects = dir;
 		infoDirectory = new File(objects, "info"); //$NON-NLS-1$
 		File packDirectory = new File(objects, "pack"); //$NON-NLS-1$
-		preservedDirectory = new File(packDirectory, "preserved"); //$NON-NLS-1$
+		File preservedDirectory = new File(packDirectory, "preserved"); //$NON-NLS-1$
 		alternatesFile = new File(objects, Constants.INFO_ALTERNATES);
 		loose = new LooseObjects(objects);
 		packed = new PackDirectory(config, packDirectory);
+		preserved = new PackDirectory(config, preservedDirectory);
 		this.fs = fs;
 		this.shallowFile = shallowFile;
 
@@ -155,7 +158,7 @@ public class ObjectDirectory extends FileObjectDatabase {
 	 * @return the location of the <code>preserved</code> directory.
 	 */
 	public final File getPreservedDirectory() {
-		return preservedDirectory;
+		return preserved.getDirectory();
 	}
 
 	/** {@inheritDoc} */
@@ -249,7 +252,13 @@ public class ObjectDirectory extends FileObjectDatabase {
 	@Override
 	public boolean has(AnyObjectId objectId) {
 		return loose.hasCached(objectId)
-				|| hasPackedInSelfOrAlternate(objectId, null)
+				|| hasPackedOrLooseInSelfOrAlternate(objectId)
+				|| (restoreFromSelfOrAlternate(objectId, null)
+						&& hasPackedOrLooseInSelfOrAlternate(objectId));
+	}
+
+	private boolean hasPackedOrLooseInSelfOrAlternate(AnyObjectId objectId) {
+		return hasPackedInSelfOrAlternate(objectId, null)
 				|| hasLooseInSelfOrAlternate(objectId, null);
 	}
 
@@ -318,6 +327,15 @@ public class ObjectDirectory extends FileObjectDatabase {
 	@Override
 	ObjectLoader openObject(WindowCursor curs, AnyObjectId objectId)
 			throws IOException {
+		ObjectLoader ldr = openObjectWithoutRestoring(curs, objectId);
+		if (ldr == null && restoreFromSelfOrAlternate(objectId, null)) {
+			ldr = openObjectWithoutRestoring(curs, objectId);
+		}
+		return ldr;
+	}
+
+	private ObjectLoader openObjectWithoutRestoring(WindowCursor curs, AnyObjectId objectId)
+			throws IOException {
 		if (loose.hasCached(objectId)) {
 			ObjectLoader ldr = openLooseObject(curs, objectId);
 			if (ldr != null) {
@@ -379,8 +397,16 @@ public class ObjectDirectory extends FileObjectDatabase {
 	}
 
 	@Override
-	long getObjectSize(WindowCursor curs, AnyObjectId id)
-			throws IOException {
+	long getObjectSize(WindowCursor curs, AnyObjectId id) throws IOException {
+		long sz = getObjectSizeWithoutRestoring(curs, id);
+		if (0 > sz && restoreFromSelfOrAlternate(id, null)) {
+			sz = getObjectSizeWithoutRestoring(curs, id);
+		}
+		return sz;
+	}
+
+	private long getObjectSizeWithoutRestoring(WindowCursor curs,
+			AnyObjectId id) throws IOException {
 		if (loose.hasCached(id)) {
 			long len = loose.getSize(curs, id);
 			if (0 <= len) {
@@ -448,6 +474,51 @@ public class ObjectDirectory extends FileObjectDatabase {
 		}
 	}
 
+	private boolean restoreFromSelfOrAlternate(AnyObjectId objectId,
+			Set<AlternateHandle.Id> skips) {
+		if (restoreFromSelf(objectId)) {
+			return true;
+		}
+
+		skips = addMe(skips);
+		for (AlternateHandle alt : myAlternates()) {
+			if (!skips.contains(alt.getId())) {
+				if (alt.db.restoreFromSelfOrAlternate(objectId, skips)) {
+					return true;
+				}
+			}
+		}
+		return false;
+	}
+
+	private boolean restoreFromSelf(AnyObjectId objectId) {
+		Pack preservedPack = preserved.getPack(objectId);
+		if (preservedPack == null) {
+			return false;
+		}
+		PackFile preservedFile = new PackFile(preservedPack.getPackFile());
+		// Restore the index last since the set will be considered for use once
+		// the index appears.
+		for (PackExt ext : PackExt.values()) {
+			if (!INDEX.equals(ext)) {
+				restore(preservedFile.create(ext));
+			}
+		}
+		restore(preservedFile.create(INDEX));
+		return true;
+	}
+
+	private boolean restore(PackFile preservedPack) {
+		PackFile restored = preservedPack
+				.createForDirectory(packed.getDirectory());
+		try {
+			Files.createLink(restored.toPath(), preservedPack.toPath());
+		} catch (IOException e) {
+			return false;
+		}
+		return true;
+	}
+
 	@Override
 	InsertLooseObjectResult insertUnpackedObject(File tmp, ObjectId id,
 			boolean createDuplicate) throws IOException {

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java

@@ -28,6 +28,7 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.errors.CorruptObjectException;
 import org.eclipse.jgit.errors.PackInvalidException;
 import org.eclipse.jgit.errors.PackMismatchException;
@@ -122,21 +123,36 @@ class PackDirectory {
 	 *
 	 * @param objectId
 	 *            identity of the object to test for existence of.
-	 * @return true if the specified object is stored in this PackDirectory.
+	 * @return {@code true} if the specified object is stored in this PackDirectory.
 	 */
 	boolean has(AnyObjectId objectId) {
+		return getPack(objectId) != null;
+	}
+
+	/**
+	 * Get the {@link org.eclipse.jgit.internal.storage.file.Pack} for the
+	 * specified object if it is stored in this PackDirectory.
+	 *
+	 * @param objectId
+	 *            identity of the object to find the Pack for.
+	 * @return {@link org.eclipse.jgit.internal.storage.file.Pack} which
+	 *         contains the specified object or {@code null} if it is not stored
+	 *         in this PackDirectory.
+	 */
+	@Nullable
+	Pack getPack(AnyObjectId objectId) {
 		PackList pList;
 		do {
 			pList = packList.get();
 			for (Pack p : pList.packs) {
 				try {
 					if (p.hasObject(objectId)) {
-						return true;
+						return p;
 					}
 				} catch (IOException e) {
-					// The hasObject call should have only touched the index,
-					// so any failure here indicates the index is unreadable
-					// by this process, and the pack is likewise not readable.
+					// The hasObject call should have only touched the index, so
+					// any failure here indicates the index is unreadable by
+					// this process, and the pack is likewise not readable.
 					LOG.warn(MessageFormat.format(
 							JGitText.get().unableToReadPackfile,
 							p.getPackFile().getAbsolutePath()), e);
@@ -144,7 +160,7 @@ class PackDirectory {
 				}
 			}
 		} while (searchPacksAgain(pList));
-		return false;
+		return null;
 	}
 
 	/**