{"thread":{"id":"62712","subject":"[PATCH] object-file: fix race in object collision check","startedAt":"2024-12-30T10:32:40Z","lastAt":"2025-01-03T07:16:53Z","messageCount":8,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509729","messageId":"20241230-b4-pks-object-file-racy-collision-check-v1-1-11571294e60a@pks.im","threadId":"62712","inReplyTo":null,"subject":"[PATCH] object-file: fix race in object collision check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T10:32:23Z","receivedAt":"2024-12-30T10:32:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"One of the tests in t5616 asserts that git-fetch(1) with `--refetch`\ntriggers repository maintenance with the correct set of arguments. This\ntest is flaky and causes us to fail sometimes:\n\n ++ git -c protocol.version=0 -c gc.autoPackLimit=0 -c maintenance.incremental-repack.auto=1234 -C pc1 fetch --refetch origin\n error: unable to open .git/objects/pack/pack-029d08823bd8a8eab510ad6ac75c823cfd3ed31e.pack: No such file or directory\n fatal: unable to rename temporary file to '.git/objects/pack/pack-029d08823bd8a8eab510ad6ac75c823cfd3ed31e.pack'\n fatal: could not finish pack-objects to repack local links\n fatal: index-pack failed\n error: last command exited with $?=128\n\nThe error message is quite confusing as it talks about trying to rename\na temporary packfile. A first hunch would thus be that this packfile\ngets written by git-fetch(1), but removed by git-maintenance(1) while it\nhasn't yet been finalized, which shouldn't ever happen. And indeed, when\nlooking closer one notices that the file that is supposedly of temporary\nnature does not have the typical `tmp_pack_` prefix.\n\nAs it turns out, the \"unable to rename temporary file\" fatal error is a\nred herring and the real error is \"unable to open\". That error is raised\nby `check_collision()`, which is called by `finalize_object_file()` when\nmoving the new packfile into place. Because t5616 re-fetches objects, we\nend up with the exact same pack as we already have in the repository. So\nwhen the concurrent git-maintenance(1) process rewrites the preexisting\npack and unlinks it exactly at the point in time where git-fetch(1)\nwants to check the old and new packfiles for equality we will see ENOENT\nand thus `check_collision()` returns an error, which gets bubbled up by\n`finalize_object_file()` and is then handled by `rename_tmp_packfile()`.\nThat function does not know about the exact root cause of the error and\ninstead just claims that the rename has failed.\n\nThis race is thus caused by b1b8dfde69 (finalize_object_file():\nimplement collision check, 2024-09-26), where we have newly introduced\nthe collision check.\n\nBy definition, two files cannot collide with each other when one of them\nhas been removed. We can thus trivially fix the issue by ignoring ENOENT\nwhen opening either of the files we're about to check for collision.\n\nSigned-off-by: Patrick Steinhardt \n---\nHi,\n\nthis patch is the follow-up for [1], where I've mentioned a couple of CI\nflakes that happen rather regularly. As it turns out, this race was a\nreal bug hiding in the newly nitroduced object collision check in case\none of the files got unlinked while performing the check.\n\nThanks!\n\nPatrick\n\n[1]: \n---\n object-file.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 5b792b3dd42cecde43a1b18abc164fd368cbcd69..f84dcd2f2a7b88716ab47bc00ee7a605a82e8d21 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1978,13 +1978,15 @@ static int check_collision(const char *filename_a, const char *filename_b)\n \n \tfd_a = open(filename_a, O_RDONLY);\n \tif (fd_a < 0) {\n-\t\tret = error_errno(_(\"unable to open %s\"), filename_a);\n+\t\tif (errno != ENOENT)\n+\t\t\tret = error_errno(_(\"unable to open %s\"), filename_a);\n \t\tgoto out;\n \t}\n \n \tfd_b = open(filename_b, O_RDONLY);\n \tif (fd_b < 0) {\n-\t\tret = error_errno(_(\"unable to open %s\"), filename_b);\n+\t\tif (errno != ENOENT)\n+\t\t\tret = error_errno(_(\"unable to open %s\"), filename_b);\n \t\tgoto out;\n \t}\n \n\n---\nbase-commit: 306ab352f4e98f6809ce52fc4e5d63fb947d0635\nchange-id: 20241230-b4-pks-object-file-racy-collision-check-62a8d1588116\n\n"},{"id":"509745","messageId":"xmqqy0zxz11m.fsf@gitster.g","threadId":"62712","inReplyTo":"20241230-b4-pks-object-file-racy-collision-check-v1-1-11571294e60a@pks.im","subject":"Re: [PATCH] object-file: fix race in object collision check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-30T14:40:53Z","receivedAt":"2024-12-30T14:40:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> By definition, two files cannot collide with each other when one of them\n> has been removed. We can thus trivially fix the issue by ignoring ENOENT\n> when opening either of the files we're about to check for collision.\n\nThanks for digging it down to the cause.\n\nIt is more like even if these two files collided (i.e. have the same\nname based on what the hash function says, with different contents),\nwhen one of them has been removed, we have no way to check if the\ncollision is benign, and even if it were not, we cannot do anything\nabout it, isn't it?\n\nI do like the simplicity of the solution. I wonder given bad enough\nrace, we could fall into a case where both files are missing?\n\nThanks, will queue.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> Hi,\n>\n> this patch is the follow-up for [1], where I've mentioned a couple of CI\n> flakes that happen rather regularly. As it turns out, this race was a\n> real bug hiding in the newly nitroduced object collision check in case\n> one of the files got unlinked while performing the check.\n>\n> Thanks!\n>\n> Patrick\n>\n> [1]: \n> ---\n> object-file.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 5b792b3dd42cecde43a1b18abc164fd368cbcd69..f84dcd2f2a7b88716ab47bc00ee7a605a82e8d21 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1978,13 +1978,15 @@ static int check_collision(const char *filename_a, const char *filename_b)\n> \n> \tfd_a = open(filename_a, O_RDONLY);\n> \tif (fd_a < 0) {\n> -\t\tret = error_errno(_(\"unable to open %s\"), filename_a);\n> +\t\tif (errno != ENOENT)\n> +\t\t\tret = error_errno(_(\"unable to open %s\"), filename_a);\n> \t\tgoto out;\n> \t}\n> \n> \tfd_b = open(filename_b, O_RDONLY);\n> \tif (fd_b < 0) {\n> -\t\tret = error_errno(_(\"unable to open %s\"), filename_b);\n> +\t\tif (errno != ENOENT)\n> +\t\t\tret = error_errno(_(\"unable to open %s\"), filename_b);\n> \t\tgoto out;\n> \t}\n> \n>\n> ---\n> base-commit: 306ab352f4e98f6809ce52fc4e5d63fb947d0635\n> change-id: 20241230-b4-pks-object-file-racy-collision-check-62a8d1588116\n"},{"id":"509746","messageId":"Z3KzHJagr_3Fkz67@pks.im","threadId":"62712","inReplyTo":"xmqqy0zxz11m.fsf@gitster.g","subject":"Re: [PATCH] object-file: fix race in object collision check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T14:50:04Z","receivedAt":"2024-12-30T14:50:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 30, 2024 at 06:40:53AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > By definition, two files cannot collide with each other when one of them\n> > has been removed. We can thus trivially fix the issue by ignoring ENOENT\n> > when opening either of the files we're about to check for collision.\n> \n> Thanks for digging it down to the cause.\n> \n> It is more like even if these two files collided (i.e. have the same\n> name based on what the hash function says, with different contents),\n> when one of them has been removed, we have no way to check if the\n> collision is benign, and even if it were not, we cannot do anything\n> about it, isn't it?\n\nDepends on what \"benign\" means in this context, I guess. We can only\nassert the most trivial case of it being \"benign\", namely that we have\ncomputed a packfile that actually is the exact same. This is also going\nto be the most common case, as everything else would depend on a\ncryptographic collision of the packfile contents. And in that case... we\ncannot do anything about it, yes.\n\n> I do like the simplicity of the solution. I wonder given bad enough\n> race, we could fall into a case where both files are missing?\n\nI was wondering about that, too, but it would very much feel like a bug\nto me if that were ever to happen. So I briefly considered whether I\nshould treat the passed-in filenames differently: \n\n - One that must exist non-racily. This is our temporary object or\n packfile that we want to move into place.\n\n - And one that may have been removed racily. This is our target file\n path that we want to overwrite, unless there is a collision.\n\nThe idea would be to only handle ENOENT for the second case. But in the\nend I don't think it's worth the complexity because `check_collision()`\nis used before rename(3p)ing the former into place, and that function\nwould already notice ENOENT anyway. So we would eventually just die the\nsame.\n\nPatrick\n"},{"id":"509747","messageId":"xmqqseq5z0f3.fsf@gitster.g","threadId":"62712","inReplyTo":"Z3KzHJagr_3Fkz67@pks.im","subject":"Re: [PATCH] object-file: fix race in object collision check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-30T14:54:24Z","receivedAt":"2024-12-30T14:54:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Dec 30, 2024 at 06:40:53AM -0800, Junio C Hamano wrote:\n>> Patrick Steinhardt writes:\n>> \n>> > By definition, two files cannot collide with each other when one of them\n>> > has been removed. We can thus trivially fix the issue by ignoring ENOENT\n>> > when opening either of the files we're about to check for collision.\n>> \n>> Thanks for digging it down to the cause.\n>> \n>> It is more like even if these two files collided (i.e. have the same\n>> name based on what the hash function says, with different contents),\n>> when one of them has been removed, we have no way to check if the\n>> collision is benign, and even if it were not, we cannot do anything\n>> about it, isn't it?\n>\n> Depends on what \"benign\" means in this context, I guess. We can only\n> assert the most trivial case of it being \"benign\", namely that we have\n> computed a packfile that actually is the exact same. This is also going\n> to be the most common case, as everything else would depend on a\n> cryptographic collision of the packfile contents. And in that case... we\n> cannot do anything about it, yes.\n\nYes, the whole point of this collision check is to notice the rare\ncase where we are under attack with cryptographic collision, so\n\"most of the time it is OK so not being able to check is fine\" is\nnot an impression we want to give readers.\n\n> The idea would be to only handle ENOENT for the second case. But in the\n> end I don't think it's worth the complexity because `check_collision()`\n> is used before rename(3p)ing the former into place, and that function\n> would already notice ENOENT anyway. So we would eventually just die the\n> same.\n\nThanks for thinking it through. Queued.\n"},{"id":"509755","messageId":"20241231014220.GA225521@coredump.intra.peff.net","threadId":"62712","inReplyTo":"Z3KzHJagr_3Fkz67@pks.im","subject":"Re: [PATCH] object-file: fix race in object collision check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-31T01:42:20Z","receivedAt":"2024-12-31T01:42:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 30, 2024 at 03:50:04PM +0100, Patrick Steinhardt wrote:\n\n> On Mon, Dec 30, 2024 at 06:40:53AM -0800, Junio C Hamano wrote:\n> > Patrick Steinhardt writes:\n> > \n> > > By definition, two files cannot collide with each other when one of them\n> > > has been removed. We can thus trivially fix the issue by ignoring ENOENT\n> > > when opening either of the files we're about to check for collision.\n> > \n> > Thanks for digging it down to the cause.\n> > \n> > It is more like even if these two files collided (i.e. have the same\n> > name based on what the hash function says, with different contents),\n> > when one of them has been removed, we have no way to check if the\n> > collision is benign, and even if it were not, we cannot do anything\n> > about it, isn't it?\n> \n> Depends on what \"benign\" means in this context, I guess. We can only\n> assert the most trivial case of it being \"benign\", namely that we have\n> computed a packfile that actually is the exact same. This is also going\n> to be the most common case, as everything else would depend on a\n> cryptographic collision of the packfile contents. And in that case... we\n> cannot do anything about it, yes.\n\nThere is one gotcha here, though. We call this collision check only if\nwe got EEXIST trying to move the tempfile into place. If the destination\nfile then goes away, we can't do the collision check. But is it right to\nquietly return success?\n\nIf the contents of the two were the same, that's fine. We don't need the\nextra copy.\n\nBut if the contents were not the same, we'd prefer either to actually\ncopy the contents into place, or to return an error.\n\nOf course we can't know, because the destination file has gone away. In\nthe common case they will be the same, but the whole point of this check\nis to allow loosening the cryptographic collision of the packfile\ncontents. So the safest thing would be to retain the tempfile, copying\nit into the destination file. That errs on the side of keeping data when\nwe cannot make a determination.\n\nIOW, if we see ENOENT on filename_b, should we then loop back in the\ncaller to try the link() again?\n\n> > I do like the simplicity of the solution. I wonder given bad enough\n> > race, we could fall into a case where both files are missing?\n> \n> I was wondering about that, too, but it would very much feel like a bug\n> to me if that were ever to happen. So I briefly considered whether I\n> should treat the passed-in filenames differently: \n> \n> - One that must exist non-racily. This is our temporary object or\n> packfile that we want to move into place.\n> \n> - And one that may have been removed racily. This is our target file\n> path that we want to overwrite, unless there is a collision.\n> \n> The idea would be to only handle ENOENT for the second case. But in the\n> end I don't think it's worth the complexity because `check_collision()`\n> is used before rename(3p)ing the former into place, and that function\n> would already notice ENOENT anyway. So we would eventually just die the\n> same.\n\nI think check_collision() is used _after_ the attempt to rename() into\nplace. So there's a race when the tempfile goes away, but I think the\noutcome is made a bit worse by your patch.\n\nConsider a sequence like this:\n\n a. Process A writes tmp_pack_foo.\n\n b. Process A tries to link tmp_pack_foo to pack- but finds it\n already exists.\n\n c. Process A opens both tmp_pack_foo and pack-.\n\n d. Process A compares the two byte-for-byte, and then returns\n success/failure based on whether they were actually identical.\n\nNow imagine there is a process B that deletes the file (maybe an\nover-zealous \"gc --prune=now\" deletes the in-use temporary file):\n\n - if process B deletes it between steps (a) and (b), process A returns\n an error (there is nothing to link). The caller knows that the data\n was not stored.\n\n - if process B deletes it between (b) and (c), then before your patch\n we see an error (because we can't compare the files). After your\n patch, we continue on and return success. The caller knows the data\n was stored (via the original file, not our new copy).\n\n - if process B deletes it between (c) and (d), then process A has no\n idea. But at this point it does not matter. If the files were\n identical, we return success (and in fact, process A deletes the file\n itself). And if not identical, then we return error, and the callers\n knows the data was not stored.\n\nSo even though the exact behavior may depend on where we hit the race, I\nthink ignoring an ENOENT open() error on the tempfile meaningfully\nchanges what happens in the middle case.\n\nIn practice I don't really expect this to happen, and \"gc --prune=now\"\nis inherently risky in a live repository. But I think we're probably\nbetter off to continue treating it as an error if we can't open our own\ntempfile.\n\n-Peff\n"},{"id":"509769","messageId":"xmqq7c7exytw.fsf@gitster.g","threadId":"62712","inReplyTo":"20241231014220.GA225521@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: fix race in object collision check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-01T16:50:51Z","receivedAt":"2025-01-01T16:50:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There is one gotcha here, though. We call this collision check only if\n> we got EEXIST trying to move the tempfile into place. If the destination\n> file then goes away, we can't do the collision check. But is it right to\n> quietly return success?\n>\n> If the contents of the two were the same, that's fine. We don't need the\n> extra copy.\n>\n> But if the contents were not the same, we'd prefer either to actually\n> copy the contents into place, or to return an error.\n>\n> Of course we can't know, because the destination file has gone away. In\n> the common case they will be the same, but the whole point of this check\n> is to allow loosening the cryptographic collision of the packfile\n> contents. So the safest thing would be to retain the tempfile, copying\n> it into the destination file. That errs on the side of keeping data when\n> we cannot make a determination.\n>\n> IOW, if we see ENOENT on filename_b, should we then loop back in the\n> caller to try the link() again?\n\nYuck, I think you're absolutely right.\n\n> I think check_collision() is used _after_ the attempt to rename() into\n> place. So there's a race when the tempfile goes away, but I think the\n> outcome is made a bit worse by your patch.\n>\n> Consider a sequence like this:\n>\n> a. Process A writes tmp_pack_foo.\n>\n> b. Process A tries to link tmp_pack_foo to pack- but finds it\n> already exists.\n>\n> c. Process A opens both tmp_pack_foo and pack-.\n>\n> d. Process A compares the two byte-for-byte, and then returns\n> success/failure based on whether they were actually identical.\n>\n> Now imagine there is a process B that deletes the file (maybe an\n> over-zealous \"gc --prune=now\" deletes the in-use temporary file):\n>\n> - if process B deletes it between steps (a) and (b), process A returns\n> an error (there is nothing to link). The caller knows that the data\n> was not stored.\n>\n> - if process B deletes it between (b) and (c), then before your patch\n> we see an error (because we can't compare the files). After your\n> patch, we continue on and return success. The caller knows the data\n> was stored (via the original file, not our new copy).\n>\n> - if process B deletes it between (c) and (d), then process A has no\n> idea. But at this point it does not matter. If the files were\n> identical, we return success (and in fact, process A deletes the file\n> itself). And if not identical, then we return error, and the callers\n> knows the data was not stored.\n>\n> So even though the exact behavior may depend on where we hit the race, I\n> think ignoring an ENOENT open() error on the tempfile meaningfully\n> changes what happens in the middle case.\n>\n> In practice I don't really expect this to happen, and \"gc --prune=now\"\n> is inherently risky in a live repository. But I think we're probably\n> better off to continue treating it as an error if we can't open our own\n> tempfile.\n\nSo we'd ignore the racy and flaky tests, as hiding the flake by\nignoring the error would only hurt the real world users.\n\n"},{"id":"509773","messageId":"20250101181952.GA1391912@coredump.intra.peff.net","threadId":"62712","inReplyTo":"xmqq7c7exytw.fsf@gitster.g","subject":"Re: [PATCH] object-file: fix race in object collision check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T18:19:52Z","receivedAt":"2025-01-01T18:20:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 01, 2025 at 08:50:51AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > There is one gotcha here, though. We call this collision check only if\n> > we got EEXIST trying to move the tempfile into place. If the destination\n> > file then goes away, we can't do the collision check. But is it right to\n> > quietly return success?\n> >\n> > If the contents of the two were the same, that's fine. We don't need the\n> > extra copy.\n> >\n> > But if the contents were not the same, we'd prefer either to actually\n> > copy the contents into place, or to return an error.\n> >\n> > Of course we can't know, because the destination file has gone away. In\n> > the common case they will be the same, but the whole point of this check\n> > is to allow loosening the cryptographic collision of the packfile\n> > contents. So the safest thing would be to retain the tempfile, copying\n> > it into the destination file. That errs on the side of keeping data when\n> > we cannot make a determination.\n> >\n> > IOW, if we see ENOENT on filename_b, should we then loop back in the\n> > caller to try the link() again?\n> \n> Yuck, I think you're absolutely right.\n\nSo I think this part, if adjusted as I suggested, would fix the race in\nthe tests without making anything worse (it's just more code).\n\nAnd then this...\n\n> > I think check_collision() is used _after_ the attempt to rename() into\n> > place. So there's a race when the tempfile goes away, but I think the\n> > outcome is made a bit worse by your patch.\n> >\n> > Consider a sequence like this:\n> >\n> > a. Process A writes tmp_pack_foo.\n> >\n> > b. Process A tries to link tmp_pack_foo to pack- but finds it\n> > already exists.\n> >\n> > c. Process A opens both tmp_pack_foo and pack-.\n> >\n> > d. Process A compares the two byte-for-byte, and then returns\n> > success/failure based on whether they were actually identical.\n> >\n> > Now imagine there is a process B that deletes the file (maybe an\n> > over-zealous \"gc --prune=now\" deletes the in-use temporary file):\n> >\n> > - if process B deletes it between steps (a) and (b), process A returns\n> > an error (there is nothing to link). The caller knows that the data\n> > was not stored.\n> >\n> > - if process B deletes it between (b) and (c), then before your patch\n> > we see an error (because we can't compare the files). After your\n> > patch, we continue on and return success. The caller knows the data\n> > was stored (via the original file, not our new copy).\n> >\n> > - if process B deletes it between (c) and (d), then process A has no\n> > idea. But at this point it does not matter. If the files were\n> > identical, we return success (and in fact, process A deletes the file\n> > itself). And if not identical, then we return error, and the callers\n> > knows the data was not stored.\n> >\n> > So even though the exact behavior may depend on where we hit the race, I\n> > think ignoring an ENOENT open() error on the tempfile meaningfully\n> > changes what happens in the middle case.\n> >\n> > In practice I don't really expect this to happen, and \"gc --prune=now\"\n> > is inherently risky in a live repository. But I think we're probably\n> > better off to continue treating it as an error if we can't open our own\n> > tempfile.\n> \n> So we'd ignore the racy and flaky tests, as hiding the flake by\n> ignoring the error would only hurt the real world users.\n\n...is all about ignoring ENOENT on the tmpfile itself. And I think we\ncan just drop that hunk entirely. The tests do not care here (they are\nrunning simultaneous gc, but _not_ a simultaneous \"--prune=now\" gc).\n\n-Peff\n"},{"id":"509825","messageId":"Z3eO2zxDOzbbFFNk@pks.im","threadId":"62712","inReplyTo":"20250101181952.GA1391912@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: fix race in object collision check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T07:16:48Z","receivedAt":"2025-01-03T07:16:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 01, 2025 at 01:19:52PM -0500, Jeff King wrote:\n> On Wed, Jan 01, 2025 at 08:50:51AM -0800, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > There is one gotcha here, though. We call this collision check only if\n> > > we got EEXIST trying to move the tempfile into place. If the destination\n> > > file then goes away, we can't do the collision check. But is it right to\n> > > quietly return success?\n> > >\n> > > If the contents of the two were the same, that's fine. We don't need the\n> > > extra copy.\n> > >\n> > > But if the contents were not the same, we'd prefer either to actually\n> > > copy the contents into place, or to return an error.\n> > >\n> > > Of course we can't know, because the destination file has gone away. In\n> > > the common case they will be the same, but the whole point of this check\n> > > is to allow loosening the cryptographic collision of the packfile\n> > > contents. So the safest thing would be to retain the tempfile, copying\n> > > it into the destination file. That errs on the side of keeping data when\n> > > we cannot make a determination.\n> > >\n> > > IOW, if we see ENOENT on filename_b, should we then loop back in the\n> > > caller to try the link() again?\n> > \n> > Yuck, I think you're absolutely right.\n> \n> So I think this part, if adjusted as I suggested, would fix the race in\n> the tests without making anything worse (it's just more code).\n\nHm, okay, I think that makes sense. Instead of being optimistic like my\nfirst version was we should be pessimistic and assume the worst. Which I\nguess is a sensible stance to take in a collision check.\n\n> And then this...\n> \n> > > I think check_collision() is used _after_ the attempt to rename() into\n> > > place. So there's a race when the tempfile goes away, but I think the\n> > > outcome is made a bit worse by your patch.\n> > >\n> > > Consider a sequence like this:\n> > >\n> > > a. Process A writes tmp_pack_foo.\n> > >\n> > > b. Process A tries to link tmp_pack_foo to pack- but finds it\n> > > already exists.\n> > >\n> > > c. Process A opens both tmp_pack_foo and pack-.\n> > >\n> > > d. Process A compares the two byte-for-byte, and then returns\n> > > success/failure based on whether they were actually identical.\n> > >\n> > > Now imagine there is a process B that deletes the file (maybe an\n> > > over-zealous \"gc --prune=now\" deletes the in-use temporary file):\n> > >\n> > > - if process B deletes it between steps (a) and (b), process A returns\n> > > an error (there is nothing to link). The caller knows that the data\n> > > was not stored.\n> > >\n> > > - if process B deletes it between (b) and (c), then before your patch\n> > > we see an error (because we can't compare the files). After your\n> > > patch, we continue on and return success. The caller knows the data\n> > > was stored (via the original file, not our new copy).\n> > >\n> > > - if process B deletes it between (c) and (d), then process A has no\n> > > idea. But at this point it does not matter. If the files were\n> > > identical, we return success (and in fact, process A deletes the file\n> > > itself). And if not identical, then we return error, and the callers\n> > > knows the data was not stored.\n> > >\n> > > So even though the exact behavior may depend on where we hit the race, I\n> > > think ignoring an ENOENT open() error on the tempfile meaningfully\n> > > changes what happens in the middle case.\n> > >\n> > > In practice I don't really expect this to happen, and \"gc --prune=now\"\n> > > is inherently risky in a live repository. But I think we're probably\n> > > better off to continue treating it as an error if we can't open our own\n> > > tempfile.\n> > \n> > So we'd ignore the racy and flaky tests, as hiding the flake by\n> > ignoring the error would only hurt the real world users.\n> \n> ...is all about ignoring ENOENT on the tmpfile itself. And I think we\n> can just drop that hunk entirely. The tests do not care here (they are\n> running simultaneous gc, but _not_ a simultaneous \"--prune=now\" gc).\n\nYeah, we don't need this hunk then.\n\nI'll send a follow-up change.\n\nPatrick\n"}]}