{"thread":{"id":"53243","subject":"Order of operations at the end of fast-import?","startedAt":"2020-04-16T04:49:32Z","lastAt":"2020-04-16T18:58:26Z","messageCount":5,"participants":["Mike Hommey","Jeff King","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"395569","messageId":"20200416042449.ztgyrdunsrzt7avp@glandium.org","threadId":"53243","inReplyTo":null,"subject":"Order of operations at the end of fast-import?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2020-04-16T04:24:49Z","receivedAt":"2020-04-16T04:49:32Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nI just noticed that the order of operations at the end of fast-import\nare:\n- end_packfile\n- dump_branches\n- dump_tags\n\nThe first may create loose objects if the pack is small (per\nfastimport.unpackLimit, defaulting to 100). The latter two create refs.\n\nThere seems to be a theoretical race condition here, if something else\ntriggers a gc at the \"wrong\" time, the loose objects might be cleaned up\nand the branches/tags refs become dangling.\n\nI understand that the packfile does need to be finished before creating\nthe refs, and that the unpacking replaces that when there aren't enough\nobjects, but wouldn't it be more data-safe to actually finish the pack,\ncreate the refs, and then unpack?\n\nMike\n"},{"id":"395573","messageId":"20200416050909.GB21547@coredump.intra.peff.net","threadId":"53243","inReplyTo":"20200416042449.ztgyrdunsrzt7avp@glandium.org","subject":"Re: Order of operations at the end of fast-import?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-16T05:09:09Z","receivedAt":"2020-04-16T05:09:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 16, 2020 at 01:24:49PM +0900, Mike Hommey wrote:\n\n> I just noticed that the order of operations at the end of fast-import\n> are:\n> - end_packfile\n> - dump_branches\n> - dump_tags\n> \n> The first may create loose objects if the pack is small (per\n> fastimport.unpackLimit, defaulting to 100). The latter two create refs.\n> \n> There seems to be a theoretical race condition here, if something else\n> triggers a gc at the \"wrong\" time, the loose objects might be cleaned up\n> and the branches/tags refs become dangling.\n>\n> I understand that the packfile does need to be finished before creating\n> the refs, and that the unpacking replaces that when there aren't enough\n> objects, but wouldn't it be more data-safe to actually finish the pack,\n> create the refs, and then unpack?\n\nThat race is there even without the unpacking step. Another gc might\nremove the pack, dropping its unreferenced objects.\n\nWe do add a \".keep\" between writing the pack and updating the refs, but\nit doesn't look like it's done atomically (i.e., we write the .idx file\nand _then_ add the .keep). So there's a small race there.\n\nBut all of this is also true of any operation, like git-commit. It's\ncreating new loose objects, and then will try to reference them. In\nbetween, a simultaneous gc will think they're unreachable. Likewise,\nreceiving a push may write a pack (with a .keep, though in the correct\norder) or even loose objects.\n\nThis is usually handled by the gc expiration time, which is compared\nagainst the file mtime. The default is 2 weeks, but even something short\nlike 5 minutes would be plenty to avoid this race (even for a long\nimport, we should be updating the mtime every time we call write()).\n\nIn fact, gc will use the same expiration for clearing out tempfiles. So\neven before we write the final pack and its .keep, any temporary files\nwe're writing into would be at risk. But again, if we're actively\nwriting, their mtimes should save them.\n\n-Peff\n"},{"id":"395579","messageId":"20200416113404.pr2hzihavdvtfjh3@glandium.org","threadId":"53243","inReplyTo":"20200416042449.ztgyrdunsrzt7avp@glandium.org","subject":"Re: Order of operations at the end of fast-import?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2020-04-16T11:34:04Z","receivedAt":"2020-04-16T11:34:21Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, Apr 16, 2020 at 01:24:49PM +0900, Mike Hommey wrote:\n> Hi,\n> \n> I just noticed that the order of operations at the end of fast-import\n> are:\n> - end_packfile\n> - dump_branches\n> - dump_tags\n> \n> The first may create loose objects if the pack is small (per\n> fastimport.unpackLimit, defaulting to 100). The latter two create refs.\n> \n> There seems to be a theoretical race condition here, if something else\n> triggers a gc at the \"wrong\" time, the loose objects might be cleaned up\n> and the branches/tags refs become dangling.\n\nCome to think of it, this is only a problem with --prune=now, and the\nsame applies to loose objects created with git add, git write-tree,\ngit-commit-tree.\n\nMike\n"},{"id":"395611","messageId":"4b3493c9-8a4b-c55e-6749-cccc9dee87b6@kdbg.org","threadId":"53243","inReplyTo":"20200416050909.GB21547@coredump.intra.peff.net","subject":"Re: Order of operations at the end of fast-import?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-04-16T18:34:27Z","receivedAt":"2020-04-16T18:34:33Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 16.04.20 um 07:09 schrieb Jeff King:\n> We do add a \".keep\" between writing the pack and updating the refs, but\n> it doesn't look like it's done atomically (i.e., we write the .idx file\n> and _then_ add the .keep). So there's a small race there.\n> \n> But all of this is also true of any operation, like git-commit. It's\n> creating new loose objects, and then will try to reference them. In\n> between, a simultaneous gc will think they're unreachable. Likewise,\n> receiving a push may write a pack (with a .keep, though in the correct\n> order) or even loose objects.\n> \n> This is usually handled by the gc expiration time, which is compared\n> against the file mtime. The default is 2 weeks, but even something short\n> like 5 minutes would be plenty to avoid this race (even for a long\n> import, we should be updating the mtime every time we call write()).\n\nExcept that on Windows the times are only updated reliably when all file\nhandles refering to the file are closed. Would that poke a hole in your\nargument? (I don't think  so, as long as expiration times are not\nridiculously short; and people who do gc --prune=now should know what\nthey are doing -- and when.)\n\n> In fact, gc will use the same expiration for clearing out tempfiles. So\n> even before we write the final pack and its .keep, any temporary files\n> we're writing into would be at risk. But again, if we're actively\n> writing, their mtimes should save them.\n\n-- Hannes\n"},{"id":"395612","messageId":"20200416185821.GA1289786@coredump.intra.peff.net","threadId":"53243","inReplyTo":"4b3493c9-8a4b-c55e-6749-cccc9dee87b6@kdbg.org","subject":"Re: Order of operations at the end of fast-import?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-16T18:58:21Z","receivedAt":"2020-04-16T18:58:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 16, 2020 at 08:34:27PM +0200, Johannes Sixt wrote:\n\n> > This is usually handled by the gc expiration time, which is compared\n> > against the file mtime. The default is 2 weeks, but even something short\n> > like 5 minutes would be plenty to avoid this race (even for a long\n> > import, we should be updating the mtime every time we call write()).\n> \n> Except that on Windows the times are only updated reliably when all file\n> handles refering to the file are closed. Would that poke a hole in your\n> argument? (I don't think  so, as long as expiration times are not\n> ridiculously short; and people who do gc --prune=now should know what\n> they are doing -- and when.)\n\nYes, if you do \"git gc --prune=5.minutes\" while an hour-long fast-import\nis running, you might be in trouble. Don't do that.\n\nThough on _any_ system, there are other cases where objects sit\nuntouched before being referenced. I guess we pull from the index for\nreachability, so \"git add\" is safe for blobs. I don't know if we are\nclever enough to use cache-tree (and it might not even be there), so\npossibly those trees are at risk until you run \"git commit\" (or even\nafter you run while waiting in the editor).\n\nCertainly I think the whole pruning system is a patchwork of assumptions\nthat could be violated in extreme cases. But it has been that way for 15\nyears. If people aren't routinely finding objects pruned out from under\nthem, I'm not inclined to spend a lot of time digging on it.\n\n-Peff\n"}]}