On Thu, Mar 09, 2023 at 10:59:12AM -0500, Jeff King wrote: > On Thu, Mar 09, 2023 at 02:09:23PM +0100, Patrick Steinhardt wrote: > > > Now in production systems we have observed that those `.keep` files are > > sometimes not getting deleted as expected, where the result is that > > repositories tend to grow packfiles that are never deleted over time. > > This seems to be caused by a race when git-receive-pack(1) is killed > > after we have migrated the kept packfile from the quarantine directory > > into the main object database. While this race window is typically small > > it can be extended for example by installing a `proc-receive` hook. > > That makes sense, and I think this is a good direction. > > > Fix this race by installing an atexit(3P) handler that unlinks the keep > > file. > > This will work if we call die(), but I think you'd be better off using > the tempfile subsystem: > > - this patch doesn't handle signal death, and I don't see any reason > you wouldn't want to handle it there (in fact, from your > description, it sounds like signal death is the culprit you suspect) > > - this will double-unlink in most cases; once when we intend to after > calling execute_commands(), and then it will try again (and > presumably fail) at exit. Probably not a huge deal, but kind of > ugly. You could set it to NULL after unlinking, but... > > - as the variable is not marked as volatile, a signal that causes an > exit could cause the handler to see an inconsistent state if you > modify it after setting up the handler. The tempfile code gets this > right and is pretty battle-tested. Ah, I didn't know that you can easily register an already-existing file as tempfile. That is indeed much nicer, thanks! > I think one could also make an argument that index_pack_lockfile() > should return a tempfile struct itself, but I didn't look too closely at > the other caller on the fetch side (but it should be conceptually the > same). I had a look at it, but git-fetch-pack(1) works quite differently in that regard as it also supports the case where the packfile lock should stay locked after it exits via the `--keep` switch. So the logic is more intricate here. Furthermore, git-fetch-pack(1) only does the locking, but never unlocks the packfiles. That is instead handled by git-fetch(1). So converting the interface to use tempfiles directly wouldn't work as we are crossing process boundaries here. And last but not least, git-fetch(1) already knows to unlock packs both via an atexit handler and via a signal handler. So there is nothing to be done here. Patrick
Attachment:
signature.asc
Description: PGP signature
