@matheusbsbs / matheus-bagliano / commits / 0ad3d65652

object-file: fix race in object collision check

One of the tests in t5616 asserts that git-fetch(1) with `--refetch` triggers repository maintenance with the correct set of arguments. This test is flaky and causes us to fail sometimes: ++ git -c protocol.version=0 -c gc.autoPackLimit=0 -c maintenance.incremental-repack.auto=1234 -C pc1 fetch --refetch origin error: unable to open .git/objects/pack/pack-029d08823bd8a8eab510ad6ac75c823cfd3ed31e.pack: No such file or directory fatal: unable to rename temporary file to '.git/objects/pack/pack-029d08823bd8a8eab510ad6ac75c823cfd3ed31e.pack' fatal: could not finish pack-objects to repack local links fatal: index-pack failed error: last command exited with $?=128 The error message is quite confusing as it talks about trying to rename a temporary packfile. A first hunch would thus be that this packfile gets written by git-fetch(1), but removed by git-maintenance(1) while it hasn't yet been finalized, which shouldn't ever happen. And indeed, when looking closer one notices that the file that is supposedly of temporary nature does not have the typical `tmp_pack_` prefix. As it turns out, the "unable to rename temporary file" fatal error is a red herring and the real error is "unable to open". That error is raised by `check_collision()`, which is called by `finalize_object_file()` when moving the new packfile into place. Because t5616 re-fetches objects, we end up with the exact same pack as we already have in the repository. So when the concurrent git-maintenance(1) process rewrites the preexisting pack and unlinks it exactly at the point in time where git-fetch(1) wants to check the old and new packfiles for equality we will see ENOENT and thus `check_collision()` returns an error, which gets bubbled up by `finalize_object_file()` and is then handled by `rename_tmp_packfile()`. That function does not know about the exact root cause of the error and instead just claims that the rename has failed. This race is thus caused by b1b8dfde69 (finalize_object_file(): implement collision check, 2024-09-26), where we have newly introduced the collision check. By definition, two files cannot collide with each other when one of them has been removed. We can thus trivially fix the issue by ignoring ENOENT when opening either of the files we're about to check for collision. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Dec 30, 2024 at 11:32 UTC 0ad3d656521aa16a6496aa855bbde97160a2b2bc

1 file changed +4 -2

object-file.c

+4 -2

@@ -1982,13 +1982,15 @@ static int check_collision(const char *filename_a, const char *filename_b)

1982

1983 fd_a = open(filename_a, O_RDONLY);

1984 if (fd_a < 0) {

1985 - ret = error_errno(_("unable to open %s"), filename_a);

1985 + if (errno != ENOENT)

1986 + ret = error_errno(_("unable to open %s"), filename_a);

1987 goto out;

1988 }

1989

1990 fd_b = open(filename_b, O_RDONLY);

1991 if (fd_b < 0) {

1991 - ret = error_errno(_("unable to open %s"), filename_b);

1992 + if (errno != ENOENT)

1993 + ret = error_errno(_("unable to open %s"), filename_b);

1994 goto out;

1995 }

1996