Ensure that GC#deleteOrphans respects pack lock
If pack or index files are guarded by a pack lock (.keep file)
deleteOrphans() should not touch the respective files protected by the
lock file. Otherwise it may interfere with PackInserter concurrently
inserting a new pack file and its index.
The problem was caused by the following race.
All mentioned files are located in "objects/pack/".
File endings relevant in "pack" dir:
.pack
.keep
.idx
.bitmap
When ReceivePack receives a pack file it executes the following steps:
ReceivePack.service():
receivePackAndCheckConnectivity():
receivePack():
receive the pack
parse the pack, returns packLock (.keep file)
PackInserter.flush():
write tmpPck file: "insert_<random>.pack"
write tmpIdx file: "insert_<random>.idx"
real pack name: "pack-<SHA1>.pack"
real index name: "pack-<SHA1>.idx"
atomic rename tmpPack to realPack
atomic rename tmpIdx to tmpIdx
execute commands
unlock pack by removing .keep file
trigger auto gc if enabled
When PackInserter.flush() renames the temporary pack to the final
"pack-xxx.pack" file the temporary pack index file "insert_xxx.idx"
has no matching .pack file with the same base name for a short interval.
If deleteOrphans() ran during that interval it deduced the pack index
file was orphaned. Subsequently the missing pack index caused
MissingObjectExceptions since objects contained in the pack couldn't be
looked up anymore.
Bug: https://bugs.chromium.org/p/gerrit/issues/detail?id=13544
Change-Id: I559c81e4b1d7c487f92a751bd78b987d32c98719
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcOrphanFilesTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcOrphanFilesTest.java
index 79d72c5..e54c4ad 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcOrphanFilesTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcOrphanFilesTest.java
@@ -57,11 +57,15 @@
private final static String BITMAP_File_1 = PACK + "-1.bitmap";
- private final static String IDX_File_2 = PACK + "-2.idx";
+ private static final String BITMAP_File_2 = PACK + "-2.bitmap";
+
+ private static final String IDX_File_2 = PACK + "-2.idx";
private final static String IDX_File_malformed = PACK + "-1234idx";
- private final static String PACK_File_2 = PACK + "-2.pack";
+ private static final String KEEP_File_2 = PACK + "-2.keep";
+
+ private static final String PACK_File_2 = PACK + "-2.pack";
private final static String PACK_File_3 = PACK + "-3.pack";
@@ -105,6 +109,22 @@
assertTrue(new File(packDir, IDX_File_malformed).exists());
}
+ @Test
+ public void keepPreventsDeletionOfIndexFilesForMissingPackFile()
+ throws Exception {
+ createFileInPackFolder(BITMAP_File_1);
+ createFileInPackFolder(IDX_File_2);
+ createFileInPackFolder(BITMAP_File_2);
+ createFileInPackFolder(KEEP_File_2);
+ createFileInPackFolder(PACK_File_3);
+ gc.gc();
+ assertFalse(new File(packDir, BITMAP_File_1).exists());
+ assertTrue(new File(packDir, BITMAP_File_2).exists());
+ assertTrue(new File(packDir, IDX_File_2).exists());
+ assertTrue(new File(packDir, KEEP_File_2).exists());
+ assertTrue(new File(packDir, PACK_File_3).exists());
+ }
+
private void createFileInPackFolder(String fileName) throws IOException {
if (!packDir.exists() || !packDir.isDirectory()) {
assertTrue(packDir.mkdirs());
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
index 791a108..25c5dcf 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
@@ -148,6 +148,8 @@
private static final String INDEX_EXT = "." + PackExt.INDEX.getExtension(); //$NON-NLS-1$
+ private static final String KEEP_EXT = "." + PackExt.KEEP.getExtension(); //$NON-NLS-1$
+
private static final int DEFAULT_AUTOPACKLIMIT = 50;
private static final int DEFAULT_AUTOLIMIT = 6700;
@@ -978,7 +980,10 @@
fileNames = files.map(path -> path.getFileName().toString())
.filter(name -> (name.endsWith(PACK_EXT)
|| name.endsWith(BITMAP_EXT)
- || name.endsWith(INDEX_EXT)))
+ || name.endsWith(INDEX_EXT)
+ || name.endsWith(KEEP_EXT)))
+ // sort files with same base name in the order:
+ // .pack, .keep, .index, .bitmap to avoid look ahead
.sorted(Collections.reverseOrder())
.collect(Collectors.toList());
} catch (IOException e1) {
@@ -990,7 +995,7 @@
String base = null;
for (String n : fileNames) {
- if (n.endsWith(PACK_EXT)) {
+ if (n.endsWith(PACK_EXT) || n.endsWith(KEEP_EXT)) {
base = n.substring(0, n.lastIndexOf('.'));
} else {
if (base == null || !n.startsWith(base)) {