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>
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java
index 61538c9..8dc1ddb 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java
@@ -23,6 +23,7 @@
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 @@
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) {
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 170df57..627facc 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
@@ -13,6 +13,7 @@
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 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 @@
private final PackDirectory packed;
- private final File preservedDirectory;
+ private final PackDirectory preserved;
private final File alternatesFile;
@@ -116,10 +118,11 @@
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 @@
* @return the location of the <code>preserved</code> directory.
*/
public final File getPreservedDirectory() {
- return preservedDirectory;
+ return preserved.getDirectory();
}
/** {@inheritDoc} */
@@ -249,7 +252,13 @@
@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 @@
@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 @@
}
@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 @@
}
}
+ 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 {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java
index 007205e..5f30970 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java
@@ -28,6 +28,7 @@
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 @@
*
* @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 @@
}
}
} while (searchPacksAgain(pList));
- return false;
+ return null;
}
/**
