Order of operations at the end of fast-import?

Hi,

I just noticed that the order of operations at the end of fast-import
are:
- end_packfile
- dump_branches
- dump_tags

The first may create loose objects if the pack is small (per fastimport.unpackLimit, defaulting to 100). The latter two create refs.

There seems to be a theoretical race condition here, if something else triggers a gc at the "wrong" time, the loose objects might be cleaned up and the branches/tags refs become dangling.

I understand that the packfile does need to be finished before creating the refs, and that the unpacking replaces that when there aren't enough objects, but wouldn't it be more data-safe to actually finish the pack, create the refs, and then unpack?

Mike

On Thu, Apr 16, 2020 at 01:24:49PM +0900, Mike Hommey wrote:

That race is there even without the unpacking step. Another gc might remove the pack, dropping its unreferenced objects.

We do add a ".keep" between writing the pack and updating the refs, but it doesn't look like it's done atomically (i.e., we write the .idx file and _then_ add the .keep). So there's a small race there.

But all of this is also true of any operation, like git-commit. It's creating new loose objects, and then will try to reference them. In between, a simultaneous gc will think they're unreachable. Likewise, receiving a push may write a pack (with a .keep, though in the correct order) or even loose objects.

This is usually handled by the gc expiration time, which is compared against the file mtime. The default is 2 weeks, but even something short like 5 minutes would be plenty to avoid this race (even for a long import, we should be updating the mtime every time we call write()).

In fact, gc will use the same expiration for clearing out tempfiles. So even before we write the final pack and its .keep, any temporary files we're writing into would be at risk. But again, if we're actively writing, their mtimes should save them.

-Peff

Am 16.04.20 um 07:09 schrieb Jeff King:

Except that on Windows the times are only updated reliably when all file handles refering to the file are closed. Would that poke a hole in your argument? (I don't think so, as long as expiration times are not ridiculously short; and people who do gc --prune=now should know what they are doing -- and when.)

-- Hannes

On Thu, Apr 16, 2020 at 08:34:27PM +0200, Johannes Sixt wrote:

Yes, if you do "git gc --prune=5.minutes" while an hour-long fast-import is running, you might be in trouble. Don't do that.

Though on _any_ system, there are other cases where objects sit untouched before being referenced. I guess we pull from the index for reachability, so "git add" is safe for blobs. I don't know if we are clever enough to use cache-tree (and it might not even be there), so possibly those trees are at risk until you run "git commit" (or even after you run while waiting in the editor).

Certainly I think the whole pruning system is a patchwork of assumptions that could be violated in extreme cases. But it has been that way for 15 years. If people aren't routinely finding objects pruned out from under them, I'm not inclined to spend a lot of time digging on it.

-Peff

On Thu, Apr 16, 2020 at 01:24:49PM +0900, Mike Hommey wrote:

Come to think of it, this is only a problem with --prune=now, and the same applies to loose objects created with git add, git write-tree, git-commit-tree.

Mike