{"thread":{"id":"54959","subject":"suspected race between packing and fetch (single case study)","startedAt":"2021-01-08T16:40:10Z","lastAt":"2021-01-13T14:56:30Z","messageCount":7,"participants":["Adina Wagner","Taylor Blau","Junio C Hamano","yoh@onerussian.com"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"413829","messageId":"e7301aaf-b341-ec0b-9e2d-ab7f60ac58da@fz-juelich.de","threadId":"54959","inReplyTo":"fe9babc8-a3ee-6be4-e4f8-9690cb7c79bd@fz-juelich.de","subject":"suspected race between packing and fetch (single case study)","fromName":"Adina Wagner","fromEmail":"a.wagner@fz-juelich.de","sentAt":"2021-01-08T16:39:12Z","receivedAt":"2021-01-08T16:40:10Z","isPatch":false,"sender":{"key":"a.wagner@fz-juelich.de","avatar":null},"body":"Hi,\n\n\ncolleagues encouraged me to report a \"personal\" bug I've stumbled\nacross. Its \"personal\", because I wasn't able to create a minimal\nreproducer, or even reproduce it with the same script on other\ninfrastructure. We're suspecting a race between packing and fetch. The\nscript I am using is at the bottom of the email.\n\nThe script creates a joint Git/git-annex repository A with a large\nnumber of objects. Afterwards, a repository B is created, and A is\ncloned into it.\nCloning fails initially. Errors look like this:\n\n+ git clone --progress ../A /tmp/B/subds\nCloning into '/tmp/B/subds'...\nfatal: failed to copy file to\n'/tmp/B/subds/.git/objects/44/93d6041a44b5a7280875ec9b6ecd78fbab7b6e':\nNo such file or directory\n\nRunning \"ps aux -H | grep git\" before and after cloning shows garbage\ncollection and packing processes in repo A. We're suspecting that there\nis a race. Here is script output that shows the processes:\n+ cd B\n+ ps aux -H\n+ grep git\nadina     674763  0.0  0.0   6152   836 pts/5    S+   16:38\n0:00           grep git\nadina     674071  0.0  0.0   9584  2788 ?        Ss   16:38   0:00\n/usr/lib/git-core/git gc --auto --no-quiet\nadina     674072  0.0  0.0   9584  3884 ?        S    16:38   0:00\n/usr/lib/git-core/git repack -d -l --no-write-bitmap-index\nadina     674073  149  0.1 583780 20564 ?        R    16:38\n0:02         /usr/lib/git-core/git pack-objects --local\n--delta-base-offset .git/objects/pack/.tmp-674072-pack\n--keep-true-parents --honor-pack-keep --non-empty --all --reflog\n--indexed-objects --unpacked  --incremental\n+ git clone --progress ../A /tmp/B/subds\nCloning into '/tmp/B/subds'...\nfatal: failed to copy file to\n'/tmp/B/subds/.git/objects/14/5a4c6775684788ecf51e5d745ac19ad5b204e3':\nNo such file or directory\n+ ps aux -H\n+ grep git\nadina     674774  0.0  0.0   6152   896 pts/5    S+   16:38\n0:00           grep git\nadina     674071  0.0  0.0   9584  2788 ?        Ss   16:38   0:00\n/usr/lib/git-core/git gc --auto --no-quiet\nadina     674072 11.0  0.0  11160  3884 ?        R    16:38   0:00\n/usr/lib/git-core/git repack -d -l --no-write-bitmap-index\nbash script.sh  65.71s user 29.53s system 94% cpu 1:40.71 total\n\n\n\nBoth A and B are completely sane repositories, git fsck shows nothing\nout of the ordinary, I can clone them fine in any situation but the\nscripted workflow. If I add a short \"sleep\" between creating A and\ncloning A into B the error vanishes.\n\nI have been able to trigger this reliably for a month with the script. I\nam running git version 2.29.2 (but also saw this when downgrading to\nversion 2.24) on Debian testing (bullseye). Other than simply waiting a\nbit before the clone, setting git config --global gc.autodetach false\nremoves the bug, too.\n\nI wonder if there is a way that Git could guard cases where background\ngc processes may still be running?\n\n\nFor completeness, here is the script I am using to trigger this on my\nmachine. We didn't manage to reproduce the behavior on another machine,\nand I didn't find a more minimal example (sorry :( ). The script\ninvolves datalad (which uses git-annex):\n\n#!/bin/sh\n\nset -x\n\n# this creates a joint git/git-annex repository\ndatalad create A && cd A\n# this adds adds and extracts a tarball with ~13.000 JPEGs to the\nrepository. Data is added to git annex.\ndatalad download-url \\\n     --archive \\\n     --message \"Download Imagenette dataset\" \\\n     'https://s3.amazonaws.com/fast-ai-imageclas/imagenette2-160.tgz'\n# this creates another joint git/git-annex repository\ncd ../ && datalad create B\ncd B\nps aux -H | grep git\ngit clone --progress ../A /tmp/B/subds\nps aux -H | grep git\n\n\nKind regards,\nAdina\n\n\n\n------------------------------------------------------------------------------------------------\n------------------------------------------------------------------------------------------------\nForschungszentrum Juelich GmbH\n52425 Juelich\nSitz der Gesellschaft: Juelich\nEingetragen im Handelsregister des Amtsgerichts Dueren Nr. HR B 3498\nVorsitzender des Aufsichtsrats: MinDir Volker Rieke\nGeschaeftsfuehrung: Prof. Dr.-Ing. Wolfgang Marquardt (Vorsitzender),\nKarsten Beneke (stellv. Vorsitzender), Prof. Dr.-Ing. Harald Bolt\n------------------------------------------------------------------------------------------------\n------------------------------------------------------------------------------------------------\n\n"},{"id":"413863","messageId":"X/ipCPFyW3gAWrHo@nand.local","threadId":"54959","inReplyTo":"e7301aaf-b341-ec0b-9e2d-ab7f60ac58da@fz-juelich.de","subject":"Re: suspected race between packing and fetch (single case study)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-08T18:48:40Z","receivedAt":"2021-01-08T18:49:36Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Adina,\n\nOn Fri, Jan 08, 2021 at 05:39:12PM +0100, Adina Wagner wrote:\n> Hi,\n>\n>\n> colleagues encouraged me to report a \"personal\" bug I've stumbled\n> across. Its \"personal\", because I wasn't able to create a minimal\n> reproducer, or even reproduce it with the same script on other\n> infrastructure. We're suspecting a race between packing and fetch. The\n> script I am using is at the bottom of the email.\n\nIndeed, similar races between fetching and repacking are known. For\nexample, this discussion:\n\n  https://lore.kernel.org/git/20200316082348.GA26581@inner.h.apk.li/\n\nis about the .idx going away during a fetch. A similar thing is\nhappening here, but instead of the .idx file going away, your source\nrepository is repacking (and thus getting rid of loose object files).\n\nHere, I think the issue is less complicated. Since you're cloning from a\nlocal repository, the 'git clone' command calls 'clone_local()', which\nin turn calls 'copy_or_link_directory()'. If the directory being copied\nchanges while being iterated over, the receiving end isn't guaranteed to\npick up the changes.\n\nWorse, if the source _removes_ a file that hasn't yet been copied, over,\nthen the copy will fail, which is what you're seeing here.\n\nOne workaround would be to clone your repositories locally with\n'--shared', which won't copy any objects from the source repository, but\ninstead mark its object store as an alternate to the newly created one.\n\n> I wonder if there is a way that Git could guard cases where background\n> gc processes may still be running?\n\nPerhaps Git could take some sort of lock when writing to the object\nstore, but an flock wouldn't work since we'd want to allow multiple\nreaders to acquire the lock simultaneously, so long as there is no\nwriter.\n\n\nThanks,\nTaylor\n"},{"id":"413934","messageId":"xmqq35z9g3pw.fsf@gitster.c.googlers.com","threadId":"54959","inReplyTo":"X/ipCPFyW3gAWrHo@nand.local","subject":"Re: suspected race between packing and fetch (single case study)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-09T22:11:55Z","receivedAt":"2021-01-09T22:12:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Here, I think the issue is less complicated. Since you're cloning from a\n> local repository, the 'git clone' command calls 'clone_local()', which\n> in turn calls 'copy_or_link_directory()'. If the directory being copied\n> changes while being iterated over, the receiving end isn't guaranteed to\n> pick up the changes.\n>\n> Worse, if the source _removes_ a file that hasn't yet been copied, over,\n> then the copy will fail, which is what you're seeing here.\n\nAnd the source that removes a file during a repack would create a\nnew file to keep the contents of the removed file available (if the\nobject still matters after the repack), but because we do not retry\nour \"cp -r\" equivalent used in the clone_local(), we may not pick\nsuch a new file up.\n\nSo, we probalby should document \"git clone --local\" that the user\nshould expect fallout similar to what may happen when they copy a\ndirectory hierarchy with \"cp -r src dst\" and muck with what is in\n\"src\" while the copy is ongoing.\n\n"},{"id":"414070","messageId":"X/ymFuUPn2POWA/p@nand.local","threadId":"54959","inReplyTo":"xmqq35z9g3pw.fsf@gitster.c.googlers.com","subject":"Re: suspected race between packing and fetch (single case study)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-11T19:25:10Z","receivedAt":"2021-01-11T19:26:05Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Jan 09, 2021 at 02:11:55PM -0800, Junio C Hamano wrote:\n> So, we probalby should document \"git clone --local\" that the user\n> should expect fallout similar to what may happen when they copy a\n> directory hierarchy with \"cp -r src dst\" and muck with what is in\n> \"src\" while the copy is ongoing.\n\nMm, good idea. Below the cut line is a patch to do just that. I thought\nbriefly about documenting it in the pack-protocol page, but it only\nmentions the local transport in passing, so it seemed inappropriate to\nadd that much more detail there.\n\n--- 8< ---\n\nSubject: [PATCH] Documentation/git-clone.txt: document race with --local\n\nWhen running 'git clone --local', the operation may fail if another\nprocess is modifying the source repository. Document that this race\ncondition is known to hopefully help anyone who may run into it.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-clone.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 876aedcd47..02d9c19cec 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -57,6 +57,10 @@ repository is specified as a URL, then this flag is ignored (and we\n never use the local optimizations).  Specifying `--no-local` will\n override the default when `/path/to/repo` is given, using the regular\n Git transport instead.\n++\n+*NOTE*: this operation can race with concurrent modification to the\n+source repository, similar to running `cp -r src dst` while modifying\n+`src`.\n\n --no-hardlinks::\n \tForce the cloning process from a repository on a local\n--\n2.30.0.138.g6d7191ea01\n\n"},{"id":"414187","messageId":"X/3gbjQs7+wHoJpb@lena.dartmouth.edu","threadId":"54959","inReplyTo":"X/ymFuUPn2POWA/p@nand.local","subject":"Re: suspected race between packing and fetch (single case study)","fromName":"","fromEmail":"yoh@onerussian.com","sentAt":"2021-01-12T17:46:22Z","receivedAt":"2021-01-12T18:18:07Z","isPatch":false,"sender":{"key":"yoh@onerussian.com","avatar":"https://gravatar.com/avatar/8901b82415ae451a83aea49409708912726e53620e3ac92320bf1f86548d97e9?d=mp&s=160"},"body":"\nOn Mon, 11 Jan 2021, Taylor Blau wrote:\n> ++\n> +*NOTE*: this operation can race with concurrent modification to the\n> +source repository, similar to running `cp -r src dst` while modifying\n> +`src`.\n\nCouldn't `gc` be triggered by git in seemingly read-only operations,\nthus possibly ruining the analogy with `cp` while doing `rm` (explicit\nintent to modify)?\n\nMoreover, situation is also a bit different since a sane user script\nwould not place `rm` into background to keep operating on original\nsource right before doing `cp` -- and that is what is happening here:\n\n`git` operation is presumably complete (but leaves `gc` running in the\nbackground) and script advances to the next step only to run into a race\ncondition with that preceding `git` command which apparently triggered\n`gc`.  Should then any script which operates on local `git` repositories\nnot to forget to add   -c gc.autodetach=0  for every git\ninvocation which might be potentially effected?\n\nCheers,\n-- \nYaroslav O. Halchenko\nCenter for Open Neuroscience     http://centerforopenneuroscience.org\nDartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755\nWWW:   http://www.linkedin.com/in/yarik        \n\n"},{"id":"414191","messageId":"X/3urtfn6L551gzJ@nand.local","threadId":"54959","inReplyTo":"X/3gbjQs7+wHoJpb@lena.dartmouth.edu","subject":"Re: suspected race between packing and fetch (single case study)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-12T18:47:10Z","receivedAt":"2021-01-12T18:48:11Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 12, 2021 at 12:46:22PM -0500, yoh@onerussian.com wrote:\n>\n> On Mon, 11 Jan 2021, Taylor Blau wrote:\n> > ++\n> > +*NOTE*: this operation can race with concurrent modification to the\n> > +source repository, similar to running `cp -r src dst` while modifying\n> > +`src`.\n>\n> Couldn't `gc` be triggered by git in seemingly read-only operations,\n> thus possibly ruining the analogy with `cp` while doing `rm` (explicit\n> intent to modify)?\n>\n> Moreover, situation is also a bit different since a sane user script\n> would not place `rm` into background to keep operating on original\n> source right before doing `cp` -- and that is what is happening here:\n\nIf you're suggesting that something is missing from the above patch, I'm\nnot sure I quite understand what you would like added.\n\nAll of these (background gc, explicit rm-ing) fall under the category of\n\"concurrent modification\": they are changing the source directory in\nsome way while a read operation is taking place.\n\n> `git` operation is presumably complete (but leaves `gc` running in the\n> background) and script advances to the next step only to run into a race\n> condition with that preceding `git` command which apparently triggered\n> `gc`.  Should then any script which operates on local `git` repositories\n> not to forget to add   -c gc.autodetach=0  for every git\n> invocation which might be potentially effected?\n\nIf your workflow is that you are frequently cloning via the local\ntransport and there is no other synchronization going on between\nwhatever work is happening in the source repository, then yes. (But note\nof course that you can set gc.autodetach=0 via the source repository's\n.git/config rather than typing it each time).\n\nThanks,\nTaylor\n"},{"id":"414265","messageId":"X/8J8ScJGL4RKBiC@lena.dartmouth.edu","threadId":"54959","inReplyTo":"X/3urtfn6L551gzJ@nand.local","subject":"Re: suspected race between packing and fetch (single case study)","fromName":"","fromEmail":"yoh@onerussian.com","sentAt":"2021-01-13T14:55:45Z","receivedAt":"2021-01-13T14:56:30Z","isPatch":false,"sender":{"key":"yoh@onerussian.com","avatar":"https://gravatar.com/avatar/8901b82415ae451a83aea49409708912726e53620e3ac92320bf1f86548d97e9?d=mp&s=160"},"body":"\nOn Tue, 12 Jan 2021, Taylor Blau wrote:\n> > > ++\n> > > +*NOTE*: this operation can race with concurrent modification to the\n> > > +source repository, similar to running `cp -r src dst` while modifying\n> > > +`src`.\n\n> > Couldn't `gc` be triggered by git in seemingly read-only operations,\n> > thus possibly ruining the analogy with `cp` while doing `rm` (explicit\n> > intent to modify)?\n\n> > Moreover, situation is also a bit different since a sane user script\n> > would not place `rm` into background to keep operating on original\n> > source right before doing `cp` -- and that is what is happening here:\n\n> If you're suggesting that something is missing from the above patch, I'm\n> not sure I quite understand what you would like added.\n\nSlept on it.  I think your patch (doc disclaimer) is factually correct\nand probably as good as it can get.  Not yet sure if it is worth\nexplicit mentioning `gc` or `repack` as one of such concurrent\noperations.\n\n> All of these (background gc, explicit rm-ing) fall under the category of\n> \"concurrent modification\": they are changing the source directory in\n> some way while a read operation is taking place.\n\nyes.  My comment was more on how such modifications are triggered: via\nexplicit actions (e.g. `rm`) intended to modify vs as a \"house\nkeeping running in the background\", which is the case of gc in\nparticular when triggered by seemingly read-only operations.\n\n> > `git` operation is presumably complete (but leaves `gc` running in the\n> > background) and script advances to the next step only to run into a race\n> > condition with that preceding `git` command which apparently triggered\n> > `gc`.  Should then any script which operates on local `git` repositories\n> > not to forget to add   -c gc.autodetach=0  for every git\n> > invocation which might be potentially effected?\n\n> If your workflow is that you are frequently cloning via the local\n> transport and there is no other synchronization going on between\n> whatever work is happening in the source repository, then yes. (But note\n> of course that you can set gc.autodetach=0 via the source repository's\n> .git/config rather than typing it each time).\n\nIMHO it affects efficiency, become cumbersome (for git users), and thus\nmight be error-prone: e.g.  gc.autodetach=0 is necessity only to\nmitigate only for a possible subsequent `clone` invocation operating\nlocally.  Higher level constructs siting on top of `git` would not know\nwhat is the next command ran in the user script (like in our case of\ndatalad) to set such config variable for their invocations.  Adding\ngc.autodetach=0 to every single `git` invocation would effect our\nefficiency. User might not be made aware of such necessity for using\n`git clone` on local repositories, only after having their scripts\ndeployed and at some random points in time start hitting the race\ncondition and go \"google\" and RTFM mode to figure out what is\ngoing on.\n\nThat is why I am more in-line with your initial comment  in\nhttps://lore.kernel.org/git/X%2FipCPFyW3gAWrHo@nand.local/ :\n\n> Perhaps Git could take some sort of lock when writing to the object\n> store, but an flock wouldn't work since we'd want to allow multiple\n> readers to acquire the lock simultaneously, so long as there is no\n> writer.\n\nI think it would be nice to have `clone_local()` first check that\nthere is no ongoing modifications happening  before proceeding and wait\nsome reasonable amount of time (up to ?0 sec?) if still ongoing, and\nthen fail \"informatively\" if still cannot clone.  Even though it would\nnot prevent race condition in full (`clone_local` might check and\ninitiate, and then some process starts altering while `clone_local` is\nongoing), it would mitigate any scripted cases of a local `git clone`\nfollowing some heavy manipulations of original repository which triggers\nbackground gc.\n\n-- \nYaroslav O. Halchenko\nCenter for Open Neuroscience     http://centerforopenneuroscience.org\nDartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755\nWWW:   http://www.linkedin.com/in/yarik        \n\n"}]}