From 60af389b49fe1d05753a4a2b9146d1b925c71e23 Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Fri, 17 Nov 2023 19:28:53 +0000 Subject: Add tests for handling pack files removal during fetch Although this could sound like a corner case, it really can occur out there in the real world. Especially in the Gerrit world where the repositories could be GC'ed on a separate process or system. The `FileNotFoundException` seems to be handled correctly in `PackFile#doOpen` (line 671) and it will mark the pack as invalid. But triggering that code path was not an easy task. First of all, we need to add a new commit to the `master` branch of the test repository after `UploadPack` object is created. Secondly, in the refspec for fetch, commit id instead of "regular" refspec must be used. With both in place, we can see a warning log statement about deleted pack file. And the fetch succeeds! Also, tests for the removal of *.idx and *.bitmap files were added. This unveiled a corner for the *.idx file deletion while fetching, as the test will fail with "Unreachable pack index" IOException only when the HEAD commit is empty. Change-Id: If26c83f9b12993d1ab7d6bad6bd863c29520b062 Signed-off-by: Dariusz Luksza (cherry picked from commit ba5adc4ce6f234cb300b0f73a1efdba9bba1b5d8) --- .../UploadPackHandleDeletedPackFileTest.java | 148 +++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java new file mode 100644 index 0000000000..b1c9447c7f --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java @@ -0,0 +1,148 @@ +/* + * Copyright (C) 2023, Dariusz Luksza and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ +package org.eclipse.jgit.transport; + +import static org.junit.Assert.fail; +import static org.eclipse.jgit.lib.Constants.HEAD; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; + +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; + +import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription; +import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; +import org.eclipse.jgit.internal.storage.file.FileRepository; +import org.eclipse.jgit.internal.storage.file.GC; +import org.eclipse.jgit.internal.storage.file.Pack; +import org.eclipse.jgit.internal.storage.pack.PackExt; +import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.junit.TestRepository.CommitBuilder; +import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.UploadPack.RequestPolicy; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +@RunWith(Parameterized.class) +public class UploadPackHandleDeletedPackFileTest + extends LocalDiskRepositoryTestCase { + + private FileRepository server; + + private TestRepository remote; + + private Repository client; + + private RevCommit head; + + @Parameter + public boolean emptyCommit; + + @Parameters(name="empty commit: {0}") + public static Collection initTestData() { + return Arrays.asList( + new Boolean[][] { { Boolean.TRUE }, { Boolean.FALSE } }); + } + + @Before + @Override + public void setUp() throws Exception { + super.setUp(); + server = createBareRepository(); + server.getConfig().setString("protocol", null, "version", "2"); + + remote = new TestRepository<>(server); + client = new InMemoryRepository(new DfsRepositoryDescription("client")); + + setupServerRepo(); + head = server.parseCommit(server.resolve(HEAD)); + } + + @Test + public void testV2PackFileRemovedDuringUploadPack() throws Exception { + doRemovePackFileDuringUploadPack(PackExt.PACK); + } + + @Test + @Ignore("pending fix") + public void testV2IdxFileRemovedDuringUploadPack() throws Exception { + doRemovePackFileDuringUploadPack(PackExt.INDEX); + } + + @Test + public void testV2BitmapFileRemovedDuringUploadPack() throws Exception { + doRemovePackFileDuringUploadPack(PackExt.BITMAP_INDEX); + } + + private void doRemovePackFileDuringUploadPack(PackExt packExt) + throws Exception { + Object ctx = new Object(); + TestProtocol testProtocol = new TestProtocol<>( + (Object req, Repository db) -> { + UploadPack up = new UploadPack(db); + up.setRequestPolicy(RequestPolicy.REACHABLE_COMMIT); + Collection packs = server.getObjectDatabase() + .getPacks(); + assertEquals("single pack expected", 1, packs.size()); + Pack pack = packs.iterator().next(); + + try { + addNewCommit(); + + new GC(remote.getRepository()).gc(); + + pack.getPackFile().create(packExt).delete(); + } catch (Exception e) { + fail("GC or pack file removal failed"); + } + + return up; + }, null); + + URIish uri = testProtocol.register(ctx, server); + + try (Transport tn = testProtocol.open(uri, client, "server")) { + tn.fetch(NullProgressMonitor.INSTANCE, + Collections.singletonList(new RefSpec(head.name()))); + assertTrue(client.getObjectDatabase().has(head)); + } + } + + private void addNewCommit() throws Exception { + CommitBuilder commit = remote.commit().message("2"); + if (!emptyCommit) { + commit = commit.add("test2.txt", remote.blob("2")); + } + remote.update("master", commit.parent(head).create()); + } + + private void setupServerRepo() throws Exception { + RevCommit commit0 = remote.commit().message("0") + .add("test.txt", remote.blob("0")) + .create(); + remote.update("master", commit0); + + new GC(remote.getRepository()).gc(); // create pack files + + head = remote.commit().message("1").parent(commit0) + .add("test1.txt", remote.blob("1")) + .create(); + remote.update("master", head); + } +} \ No newline at end of file -- cgit v1.2.3 From ca54c517647957685aa3401e097871aaebf250e3 Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Mon, 20 Nov 2023 11:00:51 +0000 Subject: Fix handling of missing pack index file As demonstrated in `UploadPackHandleDeletedPackFile.testV2IdxFileRemovedDuringUploadPack` the fetch operation will fail when the pack index file is removed. This is due to a wrapping of `FileNotFoundException` (which is a subclass of `IOExeption`) in an `IOException` at PackIndex L#68. This is then changing the behaviour of error handling in `Pack.file.getBitmapIndex()` where the `FileNotFoundException` is swallowed and allows the fetch process to continue. With FNFE being wrapped in IOE, this blows up and breaks the fetch operation. Simply rethrowing `FileNotFoundException` from `PackFile.open()` fixes the broken fetch operation. This will also mark the whole pack as invalid in the `IOException` handler in `Pack.idx()` method. Change-Id: Ibe321aa1af21d26500e1cb2eb3464cc99a6dbc62 Signed-off-by: Dariusz Luksza (cherry picked from commit e0910eda3ea1c474b4cf9b00ac698f739a982f8c) --- .../eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java | 2 -- .../src/org/eclipse/jgit/internal/storage/file/Pack.java | 6 +++--- .../src/org/eclipse/jgit/internal/storage/file/PackIndex.java | 4 +++- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java index b1c9447c7f..417ce61df2 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackHandleDeletedPackFileTest.java @@ -32,7 +32,6 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.transport.UploadPack.RequestPolicy; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -80,7 +79,6 @@ public class UploadPackHandleDeletedPackFileTest } @Test - @Ignore("pending fix") public void testV2IdxFileRemovedDuringUploadPack() throws Exception { doRemovePackFileDuringUploadPack(PackExt.INDEX); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java index 6e74136c1b..29f936587d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java @@ -1129,9 +1129,9 @@ public class Pack implements Iterable { idx = PackBitmapIndex.open(bitmapIdxFile, idx(), getReverseIdx()); } catch (FileNotFoundException e) { - // Once upon a time this bitmap file existed. Now it - // has been removed. Most likely an external gc has - // removed this packfile and the bitmap + // Once upon a time the bitmap or index files existed. Now one + // of them has been removed. Most likely an external gc has + // removed index, packfile or the bitmap bitmapIdxFile = null; return null; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackIndex.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackIndex.java index f4f62d4205..c0689dc634 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackIndex.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackIndex.java @@ -64,7 +64,9 @@ public abstract class PackIndex public static PackIndex open(File idxFile) throws IOException { try (SilentFileInputStream fd = new SilentFileInputStream( idxFile)) { - return read(fd); + return read(fd); + } catch (FileNotFoundException e) { + throw e; } catch (IOException ioe) { throw new IOException( MessageFormat.format(JGitText.get().unreadablePackIndex, -- cgit v1.2.3 From 7476183589f4c255ea561be9dff0109fc8919e9f Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Mon, 20 Nov 2023 11:53:19 +0000 Subject: Improve handling of NFS stale handle errors Mark packfile as invalid when NFS stale handle error occurs. This should fix broken fetch operations when the repo is located on the NFS system and is GC'ed on a separate system (or process). Which may result in the index, pack or bitmap file being removed when they are accessed from the fetch operation. Bug: 573770 Signed-off-by: Dariusz Luksza Change-Id: I70217bfb93bf7421ea2c1d74cbe4f15c76d9c098 (cherry picked from commit c701c01b49d92993f1c3df0a0e26a2dd68b8cec1) --- .../src/org/eclipse/jgit/internal/storage/file/Pack.java | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java index 29f936587d..d6c25b2587 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java @@ -56,6 +56,7 @@ import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectLoader; +import org.eclipse.jgit.util.FileUtils; import org.eclipse.jgit.util.LongList; import org.eclipse.jgit.util.NB; import org.eclipse.jgit.util.RawParseUtils; @@ -665,7 +666,12 @@ public class Pack implements Iterable { // exceptions signaling permanent problems with a pack openFail(true, pe); throw pe; - } catch (IOException | RuntimeException ge) { + } catch (IOException ioe) { + // mark this packfile as invalid when NFS stale file handle error + // occur + openFail(FileUtils.isStaleFileHandleInCausalChain(ioe), ioe); + throw ioe; + } catch (RuntimeException ge) { // generic exceptions could be transient so we should not mark the // pack invalid to avoid false MissingObjectExceptions openFail(false, ge); @@ -1134,6 +1140,14 @@ public class Pack implements Iterable { // removed index, packfile or the bitmap bitmapIdxFile = null; return null; + } catch (IOException e) { + if (!FileUtils.isStaleFileHandleInCausalChain(e)) { + throw e; + } + // Ignore NFS stale handle exception the same way as + // FileNotFoundException above. + bitmapIdxFile = null; + return null; } // At this point, idx() will have set packChecksum. -- cgit v1.2.3