{"thread":{"id":"65589","subject":"git hogs the CPU, RAM and storage despite its config","startedAt":"2026-05-04T15:27:35Z","lastAt":"2026-05-11T23:58:41Z","messageCount":15,"participants":["jean-christophe manciot","Jeff King","Mikael Magnusson","Taylor Blau","Derrick Stolee","Patrick Steinhardt","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"542680","messageId":"CAKcFC3arsYExb5dCMQspo4V9UFDadFaj8Q4PUsMWZJw_eYrMzA@mail.gmail.com","threadId":"65589","inReplyTo":null,"subject":"git hogs the CPU, RAM and storage despite its config","fromName":"jean-christophe manciot","fromEmail":"actionmystique@gmail.com","sentAt":"2026-05-04T15:27:21Z","receivedAt":"2026-05-04T15:27:35Z","isPatch":false,"body":"What did you do before the bug happened? (Steps to reproduce your issue)\n\nmany:\ngit add -v --force -- \"${file_path}/${file_name}\"\ngit commit -v -m \"${full_commit_message}\" -o -- \"${file_path}/${file_name}\"\n\nWhat did you expect to happen? (Expected behavior)\n\nI expected git to respect the configuration (.git/config):\n[core]\n    repositoryformatversion = 0\n    filemode = false\n    bare = false\n    logallrefupdates = true\n    fsmonitor = true\n    untrackedcache = true\n[remote \"origin\"]\n    url = git@examle.com:ppa\n    fetch = +refs/heads/*:refs/remotes/origin/*\n[branch]\n    autosetuprebase = always\n[branch \"main\"]\n    remote = origin\n    merge = refs/heads/main\n    rebase = true\n[feature]\n    manyFiles = true\n[fetch]\n    writeCommitGraph = true\n[gc]\n    auto = 0\n[pack]\n    threads = 1\n    windowMemory = 1g\n\nI expected git to use maximum one thread for packing and I'm surprised\nit even tried to perform packing as gc.auto was disabled.\n\nWhat happened instead? (Actual behavior)\n\nInstead, it used all the threads it could find (28 out of 32),\ndepriving the whole server of CPU, RAM and storage as the tmp files\nkept piling up.\n\nWhat's different between what you expected and what actually happened?\n\nUncontrollable use of CPU threads, RAM and storage\n\nAnything else you want to add:\n\nAll the threads were running:\ngit pack-objects --local --delta-base-offset --honor-pack-keep\n.git/objects/pack/.tmp-<number>-pack\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n[System Info]\ngit version:\ngit version 2.51.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 8.14.1\nzlib: 1.3.1\nSHA-1: SHA1_DC\nSHA-256: SHA256_BLK\ndefault-ref-format: files\ndefault-hash: sha1\nuname: Linux 6.17.0-23-generic #23-Ubuntu SMP PREEMPT_DYNAMIC Sat Apr\n11 23:29:57 UTC 2026 x86_64\ncompiler info: gnuc: 15.2\nlibc info: glibc: 2.42\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n-- \nJean-Christophe\n"},{"id":"542930","messageId":"20260508180341.GB737125@coredump.intra.peff.net","threadId":"65589","inReplyTo":"CAKcFC3arsYExb5dCMQspo4V9UFDadFaj8Q4PUsMWZJw_eYrMzA@mail.gmail.com","subject":"unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-08T18:03:41Z","receivedAt":"2026-05-08T18:03:42Z","isPatch":false,"body":"On Mon, May 04, 2026 at 05:27:21PM +0200, jean-christophe manciot wrote:\n\n> [gc]\n>     auto = 0\n\nThis is enough to disable auto-gc. But these days we also (instead?) run\ngit-maintenance, which is controlled by maintenance.auto. So you\nprobably are getting a bunch of background git-maintenance runs kicked\noff.\n\n> [pack]\n>     threads = 1\n>     windowMemory = 1g\n> \n> I expected git to use maximum one thread for packing and I'm surprised\n> it even tried to perform packing as gc.auto was disabled.\n\nThis should work to tell pack-objects to use only one thread, but that\nis one thread per invocation. And we were probably kicking off a ton of\nprocesses due to the background maintenance (and worse, they were all\ndoing the same work redundantly and maybe even stepping on each others\ntoes).\n\n+cc Stolee for wisdom on all things git-maintenance.\n\nShould maintenance.auto fall back to gc.auto for compatibility and\navoiding unwanted surprises when people upgrade?\n\nAlso, should background maintenance be locking to avoid multiple runs?\nIt does not seem to do so, and if I run:\n\n  git init\n  for i in $(seq 10000); do\n    echo $i >>file\n    git add file\n    git commit -m \"commit $i\"\n  done\n\nI get several concurrent pack-objects processes. After a few thousand\ncommits I got bored and hit ^C, and the resulting repo was corrupt!\nWhich is not too surprising, as multiple simultaneous repacks are known\nto be unsafe, but means we should probably avoid them.\n\n-Peff\n"},{"id":"542947","messageId":"CAHYJk3R-TyYv1MizKmHhhADrQd+VnQjxSikpcaPLB=VfHrAwpg@mail.gmail.com","threadId":"65589","inReplyTo":"20260508180341.GB737125@coredump.intra.peff.net","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Mikael Magnusson","fromEmail":"mikachu@gmail.com","sentAt":"2026-05-09T15:13:17Z","receivedAt":"2026-05-09T15:13:31Z","isPatch":false,"body":"On Fri, May 8, 2026 at 8:06 PM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, May 04, 2026 at 05:27:21PM +0200, jean-christophe manciot wrote:\n>\n> > [gc]\n> >     auto = 0\n>\n> This is enough to disable auto-gc. But these days we also (instead?) run\n> git-maintenance, which is controlled by maintenance.auto. So you\n> probably are getting a bunch of background git-maintenance runs kicked\n> off.\n>\n> > [pack]\n> >     threads = 1\n> >     windowMemory = 1g\n> >\n> > I expected git to use maximum one thread for packing and I'm surprised\n> > it even tried to perform packing as gc.auto was disabled.\n>\n> This should work to tell pack-objects to use only one thread, but that\n> is one thread per invocation. And we were probably kicking off a ton of\n> processes due to the background maintenance (and worse, they were all\n> doing the same work redundantly and maybe even stepping on each others\n> toes).\n>\n> +cc Stolee for wisdom on all things git-maintenance.\n>\n> Should maintenance.auto fall back to gc.auto for compatibility and\n> avoiding unwanted surprises when people upgrade?\n>\n> Also, should background maintenance be locking to avoid multiple runs?\n> It does not seem to do so, and if I run:\n>\n>   git init\n>   for i in $(seq 10000); do\n>     echo $i >>file\n>     git add file\n>     git commit -m \"commit $i\"\n>   done\n>\n> I get several concurrent pack-objects processes. After a few thousand\n> commits I got bored and hit ^C, and the resulting repo was corrupt!\n> Which is not too surprising, as multiple simultaneous repacks are known\n> to be unsafe, but means we should probably avoid them.\n\nThis sounds pretty horrific, this maintenance thing is enabled by\ndefault and there's nothing but pure chance that stops it from\ncorrupting the repo if I happen to run git repack manually at the same\ntime? It's hard to believe something this disruptive is even enabled\nby default, I don't expect jobs to kick off at random hours using up\nresources when I'm not working on git. (I'm assuming I'm several\nmonths late to protest this being enabled by default, but still). I\nwould strongly suggest anything like this is *never* enabled by\ndefault, it is extremely surprising to find out about. Hopefully I\nmisunderstood the part about this being enabled by default, and you\nstill need to say \"git maintenance register\", right? (although the\nmanpage erroneously(?) claims this will only enable tasks that are\nsafe, which you seem to have disproven?). The documentation is\nextremely confusing to read, it says that registering a repo for\nmaintenance will *disable* maintenance.auto in the current repo? It\nalmost seems like you guys made two entirely separate things, named\nthem both the same thing, and expected anyone to understand what's\ngoing on. git maintenance will schedule tasks to run in the background\nwith cron, and if you don't do this, the config variable named\n\"maintenance.auto\" will control if things are repacked actively while\ndoing things in the foreground? Holy moly. I'm leaving my confusion in\nplace even though I figured it out because that is something else.\n\n-- \nMikael Magnusson\n"},{"id":"542949","messageId":"20260509175249.GA2336928@coredump.intra.peff.net","threadId":"65589","inReplyTo":"20260508180341.GB737125@coredump.intra.peff.net","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-09T17:52:49Z","receivedAt":"2026-05-09T17:52:57Z","isPatch":false,"body":"On Fri, May 08, 2026 at 02:03:41PM -0400, Jeff King wrote:\n\n> Also, should background maintenance be locking to avoid multiple runs?\n> It does not seem to do so, and if I run:\n> \n>   git init\n>   for i in $(seq 10000); do\n>     echo $i >>file\n>     git add file\n>     git commit -m \"commit $i\"\n>   done\n> \n> I get several concurrent pack-objects processes. After a few thousand\n> commits I got bored and hit ^C, and the resulting repo was corrupt!\n> Which is not too surprising, as multiple simultaneous repacks are known\n> to be unsafe, but means we should probably avoid them.\n\nThere is a pretty awful bug here, and it got much worse in v2.54.\n\nFirst, the locking for \"git maintenance run --detach\" is totally broken.\nWe take the lock as the first step of maintenance_run_tasks(), then run\nforeground tasks, then daemonize(), and then run background tasks. But\ndaemonize() forks and exits the parent process, leaving the child\nprocess to do the real work. But the lock subsystem registers the\nlockfile as a tempfile to be cleaned up at exit. So the moment we\ndaemonize, we release the lock, even though we're still going to run\nsub-commands that should happen under lock.\n\nI don't think this affected the old \"git gc --detach\" because it takes\nthe lock after daemonizing[1]. We can't do the same here, though, since we\nneed to hold the lock for the foreground tasks. So either we need to\nrelease and re-take the lock between foreground and background tasks, or\nwe need to teach the daemonize() function to update the \"owner\" field on\nall of the tempfiles to the new child[2].\n\nAFAICT this has been broken since a6affd3343 (builtin/maintenance: add a\n`--detach` flag, 2024-08-16). But it was mostly harmless because under\nthe hood we were running the \"gc\" task, which ran git-gc itself in\nno-detach mode. So git-gc took its own lock, and we never ended up with\nmore than one repack running at a time.\n\nBut that changed in 452b12c2e0 (builtin/maintenance: use \"geometric\"\nstrategy by default, 2026-02-24), which is new in v2.54. Now we are\nrunning repack directly, with no additional locking. So because of the\nlock bug from a6affd3343, that means lots of repacks running at the same\ntime.\n\nUltimately fixing the lock bug will solve that. Though if doing so is\ntoo complicated for a quick maint release, I'm tempted to say we should\nconsider reverting 452b12c2e0 for a potential v2.54.1 (as there were a\nfew other regression fixes so far, I assume we'll have one soon-ish).\n\n-Peff\n\n[1] git-gc does have a mode where it will take the lock and run some\n    foreground tasks. That mode probably has the same bug, but I'm not\n    sure if it existed before we switched to \"maintenance run --auto\".\n    At this point it's moot.\n\n[2] You can't just disable the atexit handler in the parent process\n    here, because then the child wouldn't actually clean them up either\n    (because its pid will not match the owner field). So we really need\n    to transfer ownership during the daemonize() process, which probably\n    needs a new tempfile.[ch] function to walk the list and update all\n    of the owner fields.\n"},{"id":"542950","messageId":"20260509175321.GA2336838@coredump.intra.peff.net","threadId":"65589","inReplyTo":"CAHYJk3R-TyYv1MizKmHhhADrQd+VnQjxSikpcaPLB=VfHrAwpg@mail.gmail.com","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-09T17:53:21Z","receivedAt":"2026-05-09T17:53:23Z","isPatch":false,"body":"On Sat, May 09, 2026 at 05:13:17PM +0200, Mikael Magnusson wrote:\n\n> This sounds pretty horrific, this maintenance thing is enabled by\n> default and there's nothing but pure chance that stops it from\n> corrupting the repo if I happen to run git repack manually at the same\n> time? It's hard to believe something this disruptive is even enabled\n> by default, I don't expect jobs to kick off at random hours using up\n> resources when I'm not working on git. (I'm assuming I'm several\n> months late to protest this being enabled by default, but still). I\n> would strongly suggest anything like this is *never* enabled by\n> default, it is extremely surprising to find out about. Hopefully I\n> misunderstood the part about this being enabled by default, and you\n> still need to say \"git maintenance register\", right? (although the\n> manpage erroneously(?) claims this will only enable tasks that are\n> safe, which you seem to have disproven?).\n\nThis is not the time-based maintenance runs, but rather an \"auto\" mode\nthat is triggered by commands like commit when the repo state looks like\nit is in need. That mode has existed for many years, but used to be \"git\ngc --auto\". And now it is \"git maintenance run --auto\" with a new name\nand new code (and, it turns out, some new bugs).\n\nThe locking issue seems to be specific to --detach mode, so I don't\nthink affects time-based runs, only the auto mode (though if you\nregister for time-based maintenance and then manually run git-repack\noutside of \"git maintenance run\", I suspect you are subject to an\nunlikely race there).\n\nI think this did get much worse in v2.54. I left more details elsewhere\nin the thread.\n\n-Peff\n"},{"id":"542952","messageId":"af+snTGFeoUUyfPU@nand.local","threadId":"65589","inReplyTo":"20260509175249.GA2336928@coredump.intra.peff.net","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-05-09T21:52:29Z","receivedAt":"2026-05-09T21:52:35Z","isPatch":false,"body":"On Sat, May 09, 2026 at 01:52:49PM -0400, Jeff King wrote:\n> I don't think this affected the old \"git gc --detach\" because it takes\n> the lock after daemonizing[1]. We can't do the same here, though, since we\n> need to hold the lock for the foreground tasks. So either we need to\n> release and re-take the lock between foreground and background tasks, or\n> we need to teach the daemonize() function to update the \"owner\" field on\n> all of the tempfiles to the new child[2].\n\nI agree. We can't simply do what was done in 329e6e8794c (gc: save log\nfrom daemonized gc --auto and print it next time, 2015-09-19), for\nexactly the reason that you stated.\n\nDropping and re-acquiring the lock is possible, but it's racy since\nthere is a gap during the critical window while we fork(). I believe\nthat the only airtight way to do this is to update the owner field of\nthe tempfiles we want to pass down during daemonization.\n\n(As an aside, do we want to do that for all tempfiles? It might be nice\nto have a \"->reassign_on_fork\" flag or something on the tempfile struct\nin case there are instances where the parent wants to retain ownership\nof the tempfile after fork()-ing, but I can't think of any off the top\nof my head. If we do introduce such a field, it should probably default\nto \"true\" to avoid any foot-guns.)\n\n> Ultimately fixing the lock bug will solve that. Though if doing so is\n> too complicated for a quick maint release, I'm tempted to say we should\n> consider reverting 452b12c2e0 for a potential v2.54.1 (as there were a\n> few other regression fixes so far, I assume we'll have one soon-ish).\n\nI think something like the following (untested) would do the trick:\n\n--- 8< ---\ndiff --git a/setup.c b/setup.c\nindex 7ec4427368a..c07aeac4f7d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -22,6 +22,7 @@\n #include \"chdir-notify.h\"\n #include \"path.h\"\n #include \"quote.h\"\n+#include \"tempfile.h\"\n #include \"trace.h\"\n #include \"trace2.h\"\n #include \"worktree.h\"\n@@ -2162,12 +2163,17 @@ int daemonize(void)\n \terrno = ENOSYS;\n \treturn -1;\n #else\n-\tswitch (fork()) {\n+\tpid_t ppid = getpid();\n+\tpid_t pid;\n+\n+\tswitch ((pid = fork())) {\n \t\tcase 0:\n+\t\t\treassign_tempfile_ownership(ppid, getpid());\n \t\t\tbreak;\n \t\tcase -1:\n \t\t\tdie_errno(_(\"fork failed\"));\n \t\tdefault:\n+\t\t\treassign_tempfile_ownership(ppid, pid);\n \t\t\texit(0);\n \t}\n \tif (setsid() == -1)\ndiff --git a/tempfile.c b/tempfile.c\nindex 82dfa3d82f2..f0fdf582794 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -373,3 +373,15 @@ int delete_tempfile(struct tempfile **tempfile_p)\n\n \treturn err ? -1 : 0;\n }\n+\n+void reassign_tempfile_ownership(pid_t from, pid_t to)\n+{\n+\tvolatile struct volatile_list_head *pos;\n+\n+\tlist_for_each(pos, &tempfile_list) {\n+\t\tstruct tempfile *p = list_entry(pos, struct tempfile, list);\n+\n+\t\tif (is_tempfile_active(p) && p->owner == from)\n+\t\t\tp->owner = to;\n+\t}\n+}\ndiff --git a/tempfile.h b/tempfile.h\nindex 2d2ae5b657d..783d7470b54 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -282,4 +282,16 @@ int delete_tempfile(struct tempfile **tempfile_p);\n  */\n int rename_tempfile(struct tempfile **tempfile_p, const char *path);\n\n+/*\n+ * Reassign ownership of all active tempfiles whose `owner` field\n+ * matches `from` to `to`.\n+ *\n+ * This is intended for use by `daemonize()`; after `fork(2)`-ing, the\n+ * parent transfers ownership to the daemonized child so that its\n+ * atexit handler does not unlink tempfiles that should outlive it,\n+ * and the child claims the inherited tempfiles so that they are\n+ * cleaned up when the daemon exits.\n+ */\n+void reassign_tempfile_ownership(pid_t from, pid_t to);\n+\n #endif /* TEMPFILE_H */\n--- >8 ---\n\nThat's not too terrible to write, and I would feel OK about putting it\nin a 2.54.1 release soon-ish provided that others think it is reasonable.\n\nSimply reverting 452b12c2e0 (builtin/maintenance: use \"geometric\"\nstrategy by default, 2026-02-24) feels somewhat unsatisfying, since it\nis merely making the bug less likely rather than eliminating it\nentirely.\n\nSo in that sense I would prefer to \"fix forward\" here rather than to\nmask over the bug. But even the relatively short diff above is not so\nstraightforward to reason through, review, or test, so I'm open to other\nideas on how to proceed here.\n\nThanks,\nTaylor\n"},{"id":"542970","messageId":"9ddfd37d-7d71-4359-b9be-d993fbfd138c@gmail.com","threadId":"65589","inReplyTo":"af+snTGFeoUUyfPU@nand.local","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-05-10T16:08:14Z","receivedAt":"2026-05-10T16:08:17Z","isPatch":false,"body":"On 5/9/26 5:52 PM, Taylor Blau wrote:\n> On Sat, May 09, 2026 at 01:52:49PM -0400, Jeff King wrote:\n>> I don't think this affected the old \"git gc --detach\" because it takes\n>> the lock after daemonizing[1]. We can't do the same here, though, since we\n>> need to hold the lock for the foreground tasks. So either we need to\n>> release and re-take the lock between foreground and background tasks, or\n>> we need to teach the daemonize() function to update the \"owner\" field on\n>> all of the tempfiles to the new child[2].\n> \n> I agree. We can't simply do what was done in 329e6e8794c (gc: save log\n> from daemonized gc --auto and print it next time, 2015-09-19), for\n> exactly the reason that you stated.\n> \n> Dropping and re-acquiring the lock is possible, but it's racy since\n> there is a gap during the critical window while we fork(). I believe\n> that the only airtight way to do this is to update the owner field of\n> the tempfiles we want to pass down during daemonization.\n\nI agree that this drop/reacquire pattern would not be a safe choice.\n\n> (As an aside, do we want to do that for all tempfiles? It might be nice\n> to have a \"->reassign_on_fork\" flag or something on the tempfile struct\n> in case there are instances where the parent wants to retain ownership\n> of the tempfile after fork()-ing, but I can't think of any off the top\n> of my head. If we do introduce such a field, it should probably default\n> to \"true\" to avoid any foot-guns.)\n> \n>> Ultimately fixing the lock bug will solve that. Though if doing so is\n>> too complicated for a quick maint release, I'm tempted to say we should\n>> consider reverting 452b12c2e0 for a potential v2.54.1 (as there were a\n>> few other regression fixes so far, I assume we'll have one soon-ish).\n> \n> I think something like the following (untested) would do the trick:\n> \n> --- 8< ---\n> diff --git a/setup.c b/setup.c\n> index 7ec4427368a..c07aeac4f7d 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -22,6 +22,7 @@\n>   #include \"chdir-notify.h\"\n>   #include \"path.h\"\n>   #include \"quote.h\"\n> +#include \"tempfile.h\"\n>   #include \"trace.h\"\n>   #include \"trace2.h\"\n>   #include \"worktree.h\"\n> @@ -2162,12 +2163,17 @@ int daemonize(void)\n>   \terrno = ENOSYS;\n>   \treturn -1;\n>   #else\n> -\tswitch (fork()) {\n> +\tpid_t ppid = getpid();\n> +\tpid_t pid;\n> +\n> +\tswitch ((pid = fork())) {\n>   \t\tcase 0:\n> +\t\t\treassign_tempfile_ownership(ppid, getpid());\n>   \t\t\tbreak;\n>   \t\tcase -1:\n>   \t\t\tdie_errno(_(\"fork failed\"));\n>   \t\tdefault:\n> +\t\t\treassign_tempfile_ownership(ppid, pid);\n>   \t\t\texit(0);\n>   \t}\n>   \tif (setsid() == -1)\n> diff --git a/tempfile.c b/tempfile.c\n> index 82dfa3d82f2..f0fdf582794 100644\n> --- a/tempfile.c\n> +++ b/tempfile.c\n> @@ -373,3 +373,15 @@ int delete_tempfile(struct tempfile **tempfile_p)\n> \n>   \treturn err ? -1 : 0;\n>   }\n> +\n> +void reassign_tempfile_ownership(pid_t from, pid_t to)\n> +{\n> +\tvolatile struct volatile_list_head *pos;\n> +\n> +\tlist_for_each(pos, &tempfile_list) {\n> +\t\tstruct tempfile *p = list_entry(pos, struct tempfile, list);\n> +\n> +\t\tif (is_tempfile_active(p) && p->owner == from)\n> +\t\t\tp->owner = to;\n> +\t}\n> +}\n\nI worry that we're assigning _all_ tempfiles to the child in this\ncase, not just specific ones like the maintenance lock. But since\nthis is specifically called only in daemonize() then it may be\nfine.\n\nIs there any concern that since the child didn't create a tempfile\nthat the atexit() may not be initialized in the child process? Or\nwill that carry over from the fork() automatically? (This is hard\nto test without something causing a die() in the detached process.)\n> That's not too terrible to write, and I would feel OK about putting it\n> in a 2.54.1 release soon-ish provided that others think it is reasonable.\n\nI'd rather revert the geometric maintenance default for a point\nrelease and let something more complicated like this percolate\nuntil the next release window.\n\n> Simply reverting 452b12c2e0 (builtin/maintenance: use \"geometric\"\n> strategy by default, 2026-02-24) feels somewhat unsatisfying, since it\n> is merely making the bug less likely rather than eliminating it\n> entirely.\n\nIt does limit the behavior to those who previously chose to opt-in to\nthis behavior, and likely that's a low number or else we'd have more\nbug reports.\n\n> So in that sense I would prefer to \"fix forward\" here rather than to\n> mask over the bug. But even the relatively short diff above is not so\n> straightforward to reason through, review, or test, so I'm open to other\n> ideas on how to proceed here.\n\nI initially worried about cross-platform support, thinking that we\nneeded to pass file descriptors / handles and Windows always has\nissues with file handles. But we aren't actually keeping a handle\nopen but instead a record that we created the lock and should delete\nit when everything resolves.\n\nFor me to be convinced that this forward fix is the right direction,\nI'd need to see a test that proves the detached process will clean up\nthe locks on a normal process end and an early exit.\n\nThanks,\n-Stolee\n\n"},{"id":"542971","messageId":"agDjyAXxkOIso8FU@nand.local","threadId":"65589","inReplyTo":"9ddfd37d-7d71-4359-b9be-d993fbfd138c@gmail.com","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-05-10T20:00:08Z","receivedAt":"2026-05-10T20:00:14Z","isPatch":false,"body":"On Sun, May 10, 2026 at 12:08:14PM -0400, Derrick Stolee wrote:\n> I worry that we're assigning _all_ tempfiles to the child in this\n> case, not just specific ones like the maintenance lock. But since\n> this is specifically called only in daemonize() then it may be\n> fine.\n\nRight, I had wondered about this above, and I'm not sure what the right\nbehavior is here. I think in our case its fine to pass all tempfiles\ndown to the child, since we're about to exit.\n\nThe only other spot that uses `daemonize()` is 'gc --detach' and 'git\ndaemon'. I think before considering something like this we would want to\nreason through what the intended behavior of each of those callers is.\nIf we end up pursuing a fix in this direction, I think the safest thing\nthat we could do would be to have callers opt-in to this behavior so we\ncan do the right thing in git-maintenance, and other callers retain\ntheir existing behavior.\n\n> Is there any concern that since the child didn't create a tempfile\n> that the atexit() may not be initialized in the child process? Or\n> will that carry over from the fork() automatically? (This is hard\n> to test without something causing a die() in the detached process.)\n\nThe child will have its own copy of the tempfile list as well as its own\ncopy of the atexit() handlers. POSIX (via atexit(3p)) says[1] that\natexit() behaves according to the ISO C standard:\n\n    The functionality described on this reference page is aligned with\n    the ISO C standard. Any conflict between the requirements described\n    here and the ISO C standard is unintentional. This volume of\n    POSIX.1‐2017 defers to the ISO C standard.\n\n, and atexit(3) says[2]:\n\n    When a child process is created via fork(2), it inherits copies of\n    its parent's registrations.  Upon a successful call to one of the\n    exec(3) functions, all registrations are removed.\n\nSo the child has an identical copy of both the atexit() handlers and the\ntempfile list.\n\n> > That's not too terrible to write, and I would feel OK about putting it\n> > in a 2.54.1 release soon-ish provided that others think it is reasonable.\n>\n> I'd rather revert the geometric maintenance default for a point\n> release and let something more complicated like this percolate\n> until the next release window.\n>\n> > Simply reverting 452b12c2e0 (builtin/maintenance: use \"geometric\"\n> > strategy by default, 2026-02-24) feels somewhat unsatisfying, since it\n> > is merely making the bug less likely rather than eliminating it\n> > entirely.\n>\n> It does limit the behavior to those who previously chose to opt-in to\n> this behavior, and likely that's a low number or else we'd have more\n> bug reports.\n\nI agree with what you're saying. In an ideal world we would be able to\napply a fix on top that would prevent this bug from occurring, but if\nthat's not trivial to do then we shouldn't rush it in the 2.54.1 window.\n\nI think that not having this bug whatsoever (as opposed to simply\nreverting 452b12c2e0) would be preferable, but as you note we haven't\nseen any bug reports in a pre-452b12c2e0 world, so perhaps the risk is\nlow enough that we could merely revert 452b12c2e0.\n\n> For me to be convinced that this forward fix is the right direction,\n> I'd need to see a test that proves the detached process will clean up\n> the locks on a normal process end and an early exit.\n\nYeah, I agree. Testing this is a little tricky, but I played around\nwith it for a while and came up with the following:\n\n--- 8< ---\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 4700beacc18..77bfa537385 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -1460,6 +1460,56 @@ test_expect_success '--detach causes maintenance to run in background' '\n \t)\n '\n\n+test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tmkfifo fifo &&\n+\n+\t\tgit config maintenance.auto false &&\n+\t\tgit config core.lockfilepid true &&\n+\n+\t\tgit remote add origin /does/not/exist &&\n+\t\tgit config set remote.origin.uploadpack \\\n+\t\t\t\"echo \\$PPID >child && cat \\\"$(pwd)/fifo\\\"\" &&\n+\n+\t\t# Start a detached prefetch maintenance task. Note that\n+\t\t# we are backgrounding git-maintenance here in order to\n+\t\t# determine its PID to validate that the lockfile was\n+\t\t# created by the parent.\n+\t\t{ git maintenance run --task=prefetch --detach & } &&\n+\t\tparent=\"$!\" &&\n+\n+\t\t# Open fifo for writing, which will block until the\n+\t\t# upload-pack helper opens it for reading. Once exec\n+\t\t# returns, we know that the daemonized child is alive\n+\t\t# and pinned.\n+\t\texec 8>fifo &&\n+\n+\t\ttest_path_is_file .git/objects/maintenance.lock &&\n+\t\ttest_path_is_file .git/objects/maintenance~pid.lock &&\n+\n+\t\t# Verify that the maintenance.lock still exists, and\n+\t\t# that it was created by the parent process, not the\n+\t\t# child.\n+\t\techo \"pid $parent\" >expect &&\n+\t\ttest_cmp expect .git/objects/maintenance~pid.lock &&\n+\n+\t\t# Close the write end of the FIFO, causing our upload-pack\n+\t\t# helper to quit. Wait until the grandparent (from the\n+\t\t# perspective of our upload-pack helper, the daemonized\n+\t\t# git-maintenance child)\n+\t\texec 8>&- &&\n+\t\tgpid=\"$(ps -o ppid= -p $(cat child) | tr -d \" \")\" &&\n+\t\ttest -n $gpid && wait $gpid &&\n+\n+\t\ttest_path_is_missing .git/objects/maintenance.lock &&\n+\t\ttest_path_is_missing .git/objects/maintenance~pid.lock\n+\t)\n+'\n+\n test_expect_success 'repacking loose objects is quiet' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n--- >8 ---\n\nI'm not sure how portable it is, though. I think that 'ps -o ppid=' is\nOK, since 'start_git_in_background()' uses it, but I'm not sure if $PPID\nis available on Windows.\n\nIt reliably passes with the fix that I shared earlier in the thread, and\nreliably fails without it.\n\nThanks,\nTaylor\n\n[1]: https://man7.org/linux/man-pages/man3/atexit.3p.html\n[2]: https://man7.org/linux/man-pages/man3/atexit.3.html\n"},{"id":"543015","messageId":"agF6Q-z1SuTL1xXs@pks.im","threadId":"65589","inReplyTo":"agDjyAXxkOIso8FU@nand.local","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-11T06:42:11Z","receivedAt":"2026-05-11T06:42:22Z","isPatch":false,"body":"On Sun, May 10, 2026 at 04:00:08PM -0400, Taylor Blau wrote:\n> On Sun, May 10, 2026 at 12:08:14PM -0400, Derrick Stolee wrote:\n> > > That's not too terrible to write, and I would feel OK about putting it\n> > > in a 2.54.1 release soon-ish provided that others think it is reasonable.\n> >\n> > I'd rather revert the geometric maintenance default for a point\n> > release and let something more complicated like this percolate\n> > until the next release window.\n> >\n> > > Simply reverting 452b12c2e0 (builtin/maintenance: use \"geometric\"\n> > > strategy by default, 2026-02-24) feels somewhat unsatisfying, since it\n> > > is merely making the bug less likely rather than eliminating it\n> > > entirely.\n> >\n> > It does limit the behavior to those who previously chose to opt-in to\n> > this behavior, and likely that's a low number or else we'd have more\n> > bug reports.\n> \n> I agree with what you're saying. In an ideal world we would be able to\n> apply a fix on top that would prevent this bug from occurring, but if\n> that's not trivial to do then we shouldn't rush it in the 2.54.1 window.\n> \n> I think that not having this bug whatsoever (as opposed to simply\n> reverting 452b12c2e0) would be preferable, but as you note we haven't\n> seen any bug reports in a pre-452b12c2e0 world, so perhaps the risk is\n> low enough that we could merely revert 452b12c2e0.\n\nI think having an actual fix for the locking logic in git-maintenance(1)\nwould be preferable though, as the issue isn't merely contained to Git\n2.54. With that release we are of course much more likely to hit the\nissue as the \"geometric\" strategy is the default now. But even before\nthis release it was possible to configure that strategy, and if the user\nhad opted in to it they might've hit the same bug.\n\n> > For me to be convinced that this forward fix is the right direction,\n> > I'd need to see a test that proves the detached process will clean up\n> > the locks on a normal process end and an early exit.\n> \n> Yeah, I agree. Testing this is a little tricky, but I played around\n> with it for a while and came up with the following:\n> \n> --- 8< ---\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 4700beacc18..77bfa537385 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -1460,6 +1460,56 @@ test_expect_success '--detach causes maintenance to run in background' '\n>  \t)\n>  '\n> \n> +test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\tmkfifo fifo &&\n> +\n> +\t\tgit config maintenance.auto false &&\n> +\t\tgit config core.lockfilepid true &&\n> +\n> +\t\tgit remote add origin /does/not/exist &&\n> +\t\tgit config set remote.origin.uploadpack \\\n> +\t\t\t\"echo \\$PPID >child && cat \\\"$(pwd)/fifo\\\"\" &&\n> +\n> +\t\t# Start a detached prefetch maintenance task. Note that\n> +\t\t# we are backgrounding git-maintenance here in order to\n> +\t\t# determine its PID to validate that the lockfile was\n> +\t\t# created by the parent.\n> +\t\t{ git maintenance run --task=prefetch --detach & } &&\n> +\t\tparent=\"$!\" &&\n> +\n> +\t\t# Open fifo for writing, which will block until the\n> +\t\t# upload-pack helper opens it for reading. Once exec\n> +\t\t# returns, we know that the daemonized child is alive\n> +\t\t# and pinned.\n> +\t\texec 8>fifo &&\n> +\n> +\t\ttest_path_is_file .git/objects/maintenance.lock &&\n> +\t\ttest_path_is_file .git/objects/maintenance~pid.lock &&\n> +\n> +\t\t# Verify that the maintenance.lock still exists, and\n> +\t\t# that it was created by the parent process, not the\n> +\t\t# child.\n> +\t\techo \"pid $parent\" >expect &&\n> +\t\ttest_cmp expect .git/objects/maintenance~pid.lock &&\n> +\n> +\t\t# Close the write end of the FIFO, causing our upload-pack\n> +\t\t# helper to quit. Wait until the grandparent (from the\n> +\t\t# perspective of our upload-pack helper, the daemonized\n> +\t\t# git-maintenance child)\n> +\t\texec 8>&- &&\n> +\t\tgpid=\"$(ps -o ppid= -p $(cat child) | tr -d \" \")\" &&\n> +\t\ttest -n $gpid && wait $gpid &&\n> +\n> +\t\ttest_path_is_missing .git/objects/maintenance.lock &&\n> +\t\ttest_path_is_missing .git/objects/maintenance~pid.lock\n> +\t)\n> +'\n> +\n>  test_expect_success 'repacking loose objects is quiet' '\n>  \ttest_when_finished \"rm -rf repo\" &&\n>  \tgit init repo &&\n> --- >8 ---\n> \n> I'm not sure how portable it is, though. I think that 'ps -o ppid=' is\n> OK, since 'start_git_in_background()' uses it, but I'm not sure if $PPID\n> is available on Windows.\n\nYeah, this test seems to work on my system, too. But it's another\nWindows machine indeed. I'll start a CI pipeline to check whether it\nworks on Windows, as well.\n\nIn any case, I also agree with Stolee that it may have unintended\nconsequences to unconditionally reassign tempfiles to the child process\nwhen daemonizing. It is okay for all current callers, but I'm not sure\nwhether future callers would expect this behaviour.\n\nAn alternative would be to explicitly reassign the lockfile's ownership\nin git-maintenance(1). Something like the below patch.\n\nThanks!\n\nPatrick\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3a71e314c9..09cb92ac97 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n \t\t\t\t   TASK_PHASE_FOREGROUND))\n \t\t\tresult = 1;\n \n-\t/* Failure to daemonize is ok, we'll continue in foreground. */\n \tif (opts->detach > 0) {\n+\t\tpid_t child_pid;\n+\n \t\ttrace2_region_enter(\"maintenance\", \"detach\", the_repository);\n-\t\tdaemonize();\n+\n+\t\tchild_pid = daemonize_without_exit();\n+\t\tif (!child_pid) {\n+\t\t\t/*\n+\t\t\t * We're in the child process, so we take ownership of\n+\t\t\t * the lockfile.\n+\t\t\t */\n+\t\t\tlock_file_reassign_owner(&lk, getpid());\n+\t\t} else if (child_pid > 0) {\n+\t\t\t/*\n+\t\t\t * We're in the parent process, so we assign ownership\n+\t\t\t * of the lockfile to the child and then exit immediately.\n+\t\t\t */\n+\t\t\tlock_file_reassign_owner(&lk, child_pid);\n+\t\t\texit(0);\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * Failure to daemonize is ok, we'll continue in\n+\t\t\t * foreground.\n+\t\t\t */\n+\t\t}\n+\n \t\ttrace2_region_leave(\"maintenance\", \"detach\", the_repository);\n \t}\n \ndiff --git a/lockfile.c b/lockfile.c\nindex 7add2f136a..96aab3c885 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -356,3 +356,12 @@ int rollback_lock_file(struct lock_file *lk)\n \tdelete_tempfile(&lk->pid_tempfile);\n \treturn delete_tempfile(&lk->tempfile);\n }\n+\n+void lock_file_reassign_owner(struct lock_file *lk, pid_t owner)\n+{\n+\tif (!is_lock_file_locked(lk))\n+\t\tBUG(\"cannot reassign ownership of unlocked lockfile\");\n+\tlk->tempfile->owner = owner;\n+\tif (lk->pid_tempfile)\n+\t\tlk->pid_tempfile->owner = owner;\n+}\ndiff --git a/lockfile.h b/lockfile.h\nindex e7233f28de..0b10b624fa 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -341,4 +341,14 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)\n  */\n int rollback_lock_file(struct lock_file *lk);\n \n+/*\n+ * Reassign ownership of the lockfile to a different process.\n+ *\n+ * This is intended for use after `fork(2)`-ing. The parent transfers ownership\n+ * to the daemonized child so that its atexit handler does not unlink the lock\n+ * that should outlive it, and the child claims the inherited tempfiles so that\n+ * they are cleaned up when the daemon exits.\n+ */\n+void lock_file_reassign_owner(struct lock_file *lk, pid_t owner);\n+\n #endif /* LOCKFILE_H */\ndiff --git a/setup.c b/setup.c\nindex 7ec4427368..d651e11d4b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2156,20 +2156,18 @@ void sanitize_stdfds(void)\n \t\tclose(fd);\n }\n \n-int daemonize(void)\n+pid_t daemonize_without_exit(void)\n {\n #ifdef NO_POSIX_GOODIES\n \terrno = ENOSYS;\n \treturn -1;\n #else\n-\tswitch (fork()) {\n-\t\tcase 0:\n-\t\t\tbreak;\n-\t\tcase -1:\n-\t\t\tdie_errno(_(\"fork failed\"));\n-\t\tdefault:\n-\t\t\texit(0);\n-\t}\n+\tpid_t pid = fork();\n+\tif (pid < 0)\n+\t\treturn error_errno(_(\"fork failed\"));\n+\tif (pid > 0)\n+\t\treturn pid;\n+\n \tif (setsid() == -1)\n \t\tdie_errno(_(\"setsid failed\"));\n \tclose(0);\n@@ -2180,6 +2178,21 @@ int daemonize(void)\n #endif\n }\n \n+int daemonize(void)\n+{\n+#ifdef NO_POSIX_GOODIES\n+\terrno = ENOSYS;\n+\treturn -1;\n+#else\n+\tpid_t pid = daemonize_without_exit();\n+\tif (pid < 0)\n+\t\tdie(NULL);\n+\tif (pid > 0)\n+\t\texit(0);\n+\treturn 0;\n+#endif\n+}\n+\n struct template_dir_cb_data {\n \tchar *path;\n \tint initialized;\ndiff --git a/setup.h b/setup.h\nindex 80bc6e5f07..10f8b377f2 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -150,6 +150,7 @@ int path_inside_repo(const char *prefix, const char *path);\n \n void sanitize_stdfds(void);\n int daemonize(void);\n+int daemonize_without_exit(void);\n \n /*\n  * GIT_REPO_VERSION is the version we write by default. The\n"},{"id":"543088","messageId":"20260511200112.GA22912@coredump.intra.peff.net","threadId":"65589","inReplyTo":"af+snTGFeoUUyfPU@nand.local","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-11T20:01:12Z","receivedAt":"2026-05-11T20:01:13Z","isPatch":false,"body":"On Sat, May 09, 2026 at 05:52:29PM -0400, Taylor Blau wrote:\n\n> Dropping and re-acquiring the lock is possible, but it's racy since\n> there is a gap during the critical window while we fork(). I believe\n> that the only airtight way to do this is to update the owner field of\n> the tempfiles we want to pass down during daemonization.\n\nIt's racy in the sense that we might not get the lock back, but I don't\nthink that's _wrong_ exactly. We just care that some instance runs the\nsecond half eventually.\n\nIt is technically vulnerable to starvation, if processes keep taking the\nlock, doing the foreground jobs, and then losing the race to take it a\nsecond time. But besides the fact that you'd eventually win by chance,\nunless you have an infinite supply of processes coming along to run the\nforeground half, the final invocation will run the background tasks.\n\nI agree it feels pretty hacky. It is, however, how \"git gc\" seems to\nwork (notice that it does a manual delete_tempfile() on the lock right\nbefore daemonizing).\n\n> (As an aside, do we want to do that for all tempfiles? It might be nice\n> to have a \"->reassign_on_fork\" flag or something on the tempfile struct\n> in case there are instances where the parent wants to retain ownership\n> of the tempfile after fork()-ing, but I can't think of any off the top\n> of my head. If we do introduce such a field, it should probably default\n> to \"true\" to avoid any foot-guns.)\n\nI think the answer is yes, we want it for all tempfiles. Because it is\nnot a property of the individual tempfiles, but rather of the caller who\nis forking. The point of that owner field is so that when we spawn\nchildren, we don't end up with duplicate or unexpected cleanups.\n\nBut daemonize() is not about spawning a child or otherwise splitting the\nprocess into two. There is still a single contiguous flow of execution,\nand the fact that we switch pids and call exit() is an implementation\ndetail. So we would still want to remain the \"owner\" of any cleanup.\n\nAnd that is true for any other atexit() handlers besides the tempfile\nones, too. If we called run_processes_parallel(), and then while that\nchild was running we called daemonize(), we'd not want to kill the child\nthen, but our atexit handler would do so! But we don't tend to leave\nprocesses running while doing stuff like daemonize(), which is why we\nnever bothered to implement an owner field there in the first place.\n\n> > Ultimately fixing the lock bug will solve that. Though if doing so is\n> > too complicated for a quick maint release, I'm tempted to say we should\n> > consider reverting 452b12c2e0 for a potential v2.54.1 (as there were a\n> > few other regression fixes so far, I assume we'll have one soon-ish).\n> \n> I think something like the following (untested) would do the trick:\n\nYeah, that's what I was envisioning. Note that there's a small race\nhere:\n\n> +\tswitch ((pid = fork())) {\n>  \t\tcase 0:\n> +\t\t\treassign_tempfile_ownership(ppid, getpid());\n>  \t\t\tbreak;\n>  \t\tcase -1:\n>  \t\t\tdie_errno(_(\"fork failed\"));\n>  \t\tdefault:\n> +\t\t\treassign_tempfile_ownership(ppid, pid);\n>  \t\t\texit(0);\n>  \t}\n\nThe parent is giving up ownership and the child is taking it. So there\nmay be a moment where both claim ownership, or a moment where the parent\nhas relinquished but the child has not yet taken. We're not going to\ncall exit() in the middle there, but a signal could kill us.\n\nAnd then either:\n\n  - If both claim ownership, it's mostly OK, because they'll both try to\n    unlink() which is idempotent-ish. Though there is a bad sequence\n    where we delete somebody _else's_ lock, like:\n\n       1. Parent forks, but has not yet reassigned.\n\n       2. Child calls reassign to take ownership. Now both have\n\t  ownership.\n\n       3. Signal kills both parent and child, so they enter cleanup\n\t  code.\n\n       4. One of them (let's say the parent) deletes the lockfile.\n\n       5. Some other unrelated process (let's call it \"git other\") takes\n\t  the lock.\n\n       6. The child deletes the lockfile.\n\n    And at that point \"git other\" thinks it holds the lock, but it\n    doesn't. It's quite an unlikely sequence in practice, though, I'd\n    think.\n\n  - If neither claims ownership and a signal kills both, then nobody\n    cleans up the lock and it is left in place. This is annoying, but\n    also something that can happen occasionally anyway (kill -9, etc).\n\nI don't have an easy suggestion for making it more atomic, though. You\ncould choose one or the other direction using some synchronization\nbetween the two (e.g., child reassigns only after parent signals over a\npipe that it has relinquished), but it's all kind of ugly.\n\nSo it may be reasonable to just shrug and assume it's unlikely enough\nnot to worry about.\n\n-Peff\n"},{"id":"543089","messageId":"20260511201049.GB22912@coredump.intra.peff.net","threadId":"65589","inReplyTo":"9ddfd37d-7d71-4359-b9be-d993fbfd138c@gmail.com","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-11T20:10:49Z","receivedAt":"2026-05-11T20:10:50Z","isPatch":false,"body":"On Sun, May 10, 2026 at 12:08:14PM -0400, Derrick Stolee wrote:\n\n> > So in that sense I would prefer to \"fix forward\" here rather than to\n> > mask over the bug. But even the relatively short diff above is not so\n> > straightforward to reason through, review, or test, so I'm open to other\n> > ideas on how to proceed here.\n> \n> I initially worried about cross-platform support, thinking that we\n> needed to pass file descriptors / handles and Windows always has\n> issues with file handles. But we aren't actually keeping a handle\n> open but instead a record that we created the lock and should delete\n> it when everything resolves.\n\nThe daemonize() function is a noop on Windows anyway, because we don't\nhave fork(). It's controlled by the NO_POSIX_GOODIES knob.\n\n> For me to be convinced that this forward fix is the right direction,\n> I'd need to see a test that proves the detached process will clean up\n> the locks on a normal process end and an early exit.\n\nI think it's hard to trigger an early exit, since the only thing we do\nwhile holding the lock is run the maintenance tasks, and those are\nalmost entirely just using run-command to run subprocesses. So I don't\nsee a single path that can lead to a die().\n\nWhich leaves signals. It is easy-ish to test manually with a\nlong-running maintenance (say, the \"gc\" task on linux.git), verifying\nthat maintenance.lock is there, killing the detached git-maintenance\nprocess with SIGTERM, and then confirming that the lock was cleaned up.\n\nIt looks like Taylor found a way to make an arbitrarily slow task using\nthe prefetch process. So I think his test could be modified to issue a\nkill at the right moment and verify cleanup (rather than waiting for a\nnormal exit, which calls rollback_lock_file() and never checks the owner\nfield at all).\n\n-Peff\n"},{"id":"543091","messageId":"590781db-7c19-4aa8-8497-e16e5eb5eba1@intel.com","threadId":"65589","inReplyTo":"20260511200112.GA22912@coredump.intra.peff.net","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2026-05-11T20:21:37Z","receivedAt":"2026-05-11T20:21:48Z","isPatch":false,"body":"On 5/11/2026 1:01 PM, Jeff King wrote:\n> On Sat, May 09, 2026 at 05:52:29PM -0400, Taylor Blau wrote:\n> \n>> Dropping and re-acquiring the lock is possible, but it's racy since\n>> there is a gap during the critical window while we fork(). I believe\n>> that the only airtight way to do this is to update the owner field of\n>> the tempfiles we want to pass down during daemonization.\n> \n> It's racy in the sense that we might not get the lock back, but I don't\n> think that's _wrong_ exactly. We just care that some instance runs the\n> second half eventually.\n> \n> It is technically vulnerable to starvation, if processes keep taking the\n> lock, doing the foreground jobs, and then losing the race to take it a\n> second time. But besides the fact that you'd eventually win by chance,\n> unless you have an infinite supply of processes coming along to run the\n> foreground half, the final invocation will run the background tasks.\n> \n> I agree it feels pretty hacky. It is, however, how \"git gc\" seems to\n> work (notice that it does a manual delete_tempfile() on the lock right\n> before daemonizing).\n> \n>> (As an aside, do we want to do that for all tempfiles? It might be nice\n>> to have a \"->reassign_on_fork\" flag or something on the tempfile struct\n>> in case there are instances where the parent wants to retain ownership\n>> of the tempfile after fork()-ing, but I can't think of any off the top\n>> of my head. If we do introduce such a field, it should probably default\n>> to \"true\" to avoid any foot-guns.)\n> \n> I think the answer is yes, we want it for all tempfiles. Because it is\n> not a property of the individual tempfiles, but rather of the caller who\n> is forking. The point of that owner field is so that when we spawn\n> children, we don't end up with duplicate or unexpected cleanups.\n> \n> But daemonize() is not about spawning a child or otherwise splitting the\n> process into two. There is still a single contiguous flow of execution,\n> and the fact that we switch pids and call exit() is an implementation\n> detail. So we would still want to remain the \"owner\" of any cleanup.\n> \n> And that is true for any other atexit() handlers besides the tempfile\n> ones, too. If we called run_processes_parallel(), and then while that\n> child was running we called daemonize(), we'd not want to kill the child\n> then, but our atexit handler would do so! But we don't tend to leave\n> processes running while doing stuff like daemonize(), which is why we\n> never bothered to implement an owner field there in the first place.\n> \n>>> Ultimately fixing the lock bug will solve that. Though if doing so is\n>>> too complicated for a quick maint release, I'm tempted to say we should\n>>> consider reverting 452b12c2e0 for a potential v2.54.1 (as there were a\n>>> few other regression fixes so far, I assume we'll have one soon-ish).\n>>\n>> I think something like the following (untested) would do the trick:\n> \n> Yeah, that's what I was envisioning. Note that there's a small race\n> here:\n> \n>> +\tswitch ((pid = fork())) {\n>>  \t\tcase 0:\n>> +\t\t\treassign_tempfile_ownership(ppid, getpid());\n>>  \t\t\tbreak;\n>>  \t\tcase -1:\n>>  \t\t\tdie_errno(_(\"fork failed\"));\n>>  \t\tdefault:\n>> +\t\t\treassign_tempfile_ownership(ppid, pid);\n>>  \t\t\texit(0);\n>>  \t}\n> \n> The parent is giving up ownership and the child is taking it. So there\n> may be a moment where both claim ownership, or a moment where the parent\n> has relinquished but the child has not yet taken. We're not going to\n> call exit() in the middle there, but a signal could kill us.\n> \n> And then either:\n> \n>   - If both claim ownership, it's mostly OK, because they'll both try to\n>     unlink() which is idempotent-ish. Though there is a bad sequence\n>     where we delete somebody _else's_ lock, like:\n> \n>        1. Parent forks, but has not yet reassigned.\n> \n>        2. Child calls reassign to take ownership. Now both have\n> \t  ownership.\n> \n>        3. Signal kills both parent and child, so they enter cleanup\n> \t  code.\n> \n>        4. One of them (let's say the parent) deletes the lockfile.\n> \n>        5. Some other unrelated process (let's call it \"git other\") takes\n> \t  the lock.\n> \n>        6. The child deletes the lockfile.\n> \n>     And at that point \"git other\" thinks it holds the lock, but it\n>     doesn't. It's quite an unlikely sequence in practice, though, I'd\n>     think.\n> \n>   - If neither claims ownership and a signal kills both, then nobody\n>     cleans up the lock and it is left in place. This is annoying, but\n>     also something that can happen occasionally anyway (kill -9, etc).\n> \n> I don't have an easy suggestion for making it more atomic, though. You\n> could choose one or the other direction using some synchronization\n> between the two (e.g., child reassigns only after parent signals over a\n> pipe that it has relinquished), but it's all kind of ugly.\n> \nYa, that seems really ugly. My first thought was some way to disable\nsignals temporarily, but I am guessing that either has no good way to do\nit or would introduce even more issues. Plus there is always kill -9...\n\n> So it may be reasonable to just shrug and assume it's unlikely enough\n> not to worry about.\n> \n> -Peff\n> \n\nYea, I agree that this might qualify as unlikely enough to not worry.\n\n"},{"id":"543092","messageId":"20260511202258.GD22912@coredump.intra.peff.net","threadId":"65589","inReplyTo":"agF6Q-z1SuTL1xXs@pks.im","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-11T20:22:58Z","receivedAt":"2026-05-11T20:22:59Z","isPatch":false,"body":"On Mon, May 11, 2026 at 08:42:11AM +0200, Patrick Steinhardt wrote:\n\n> In any case, I also agree with Stolee that it may have unintended\n> consequences to unconditionally reassign tempfiles to the child process\n> when daemonizing. It is okay for all current callers, but I'm not sure\n> whether future callers would expect this behaviour.\n> \n> An alternative would be to explicitly reassign the lockfile's ownership\n> in git-maintenance(1). Something like the below patch.\n\nI really think this should be an automatic part of daemonize(), for the\nreasons I gave earlier in the thread. It's really a lurking problem for\nany cleanup handler, but lockfiles are the one that is most likely to\nbite us.\n\nIn fact, I thought \"git gc --detach\" without \"--skip-foreground-tasks\"\nhad the same bug, but it narrowly avoids it by dropping and reacquiring\nthe lock (!) on either side of the daemonize() call.\n\nSo I think you are more likely to avoid unpleasant surprises by baking\nthe behavior into daemonize() rather than requiring each caller to do it\nmanually.\n\nPlus it keeps daemonize() a bit more abstract. It's a noop on Windows,\nbut you can imagine an implementation that does weird platform-specific\nstuff and doesn't switch pids at all.\n\n-Peff\n"},{"id":"543093","messageId":"20260511203502.GA25510@coredump.intra.peff.net","threadId":"65589","inReplyTo":"590781db-7c19-4aa8-8497-e16e5eb5eba1@intel.com","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-11T20:35:02Z","receivedAt":"2026-05-11T20:35:05Z","isPatch":false,"body":"On Mon, May 11, 2026 at 01:21:37PM -0700, Jacob Keller wrote:\n\n> >   - If both claim ownership, it's mostly OK, because they'll both try to\n> >     unlink() which is idempotent-ish. Though there is a bad sequence\n> >     where we delete somebody _else's_ lock, like:\n> > \n> >        1. Parent forks, but has not yet reassigned.\n> > \n> >        2. Child calls reassign to take ownership. Now both have\n> > \t  ownership.\n> > \n> >        3. Signal kills both parent and child, so they enter cleanup\n> > \t  code.\n> > \n> >        4. One of them (let's say the parent) deletes the lockfile.\n> > \n> >        5. Some other unrelated process (let's call it \"git other\") takes\n> > \t  the lock.\n> > \n> >        6. The child deletes the lockfile.\n> > \n> >     And at that point \"git other\" thinks it holds the lock, but it\n> >     doesn't. It's quite an unlikely sequence in practice, though, I'd\n> >     think.\n> > \n> >   - If neither claims ownership and a signal kills both, then nobody\n> >     cleans up the lock and it is left in place. This is annoying, but\n> >     also something that can happen occasionally anyway (kill -9, etc).\n> > \n> > I don't have an easy suggestion for making it more atomic, though. You\n> > could choose one or the other direction using some synchronization\n> > between the two (e.g., child reassigns only after parent signals over a\n> > pipe that it has relinquished), but it's all kind of ugly.\n> > \n> Ya, that seems really ugly. My first thought was some way to disable\n> signals temporarily, but I am guessing that either has no good way to do\n> it or would introduce even more issues. Plus there is always kill -9...\n\nI guess the parent could relinquish control by reassigning to \"0\" before\neven calling fork(), and then taking it back if fork() fails. And then\nworst case is that nobody cleans up the lock (if the child is killed\nbefore taking control), but that's the least-bad outcome from a\ncorrectness perspective (though still annoying).\n\n-Peff\n"},{"id":"543099","messageId":"d769b895-4388-4c5d-bc13-52bc80ca6b01@intel.com","threadId":"65589","inReplyTo":"20260511203502.GA25510@coredump.intra.peff.net","subject":"Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2026-05-11T23:58:34Z","receivedAt":"2026-05-11T23:58:41Z","isPatch":false,"body":"On 5/11/2026 1:35 PM, Jeff King wrote:\n> On Mon, May 11, 2026 at 01:21:37PM -0700, Jacob Keller wrote:\n> \n>>>   - If both claim ownership, it's mostly OK, because they'll both try to\n>>>     unlink() which is idempotent-ish. Though there is a bad sequence\n>>>     where we delete somebody _else's_ lock, like:\n>>>\n>>>        1. Parent forks, but has not yet reassigned.\n>>>\n>>>        2. Child calls reassign to take ownership. Now both have\n>>> \t  ownership.\n>>>\n>>>        3. Signal kills both parent and child, so they enter cleanup\n>>> \t  code.\n>>>\n>>>        4. One of them (let's say the parent) deletes the lockfile.\n>>>\n>>>        5. Some other unrelated process (let's call it \"git other\") takes\n>>> \t  the lock.\n>>>\n>>>        6. The child deletes the lockfile.\n>>>\n>>>     And at that point \"git other\" thinks it holds the lock, but it\n>>>     doesn't. It's quite an unlikely sequence in practice, though, I'd\n>>>     think.\n>>>\n>>>   - If neither claims ownership and a signal kills both, then nobody\n>>>     cleans up the lock and it is left in place. This is annoying, but\n>>>     also something that can happen occasionally anyway (kill -9, etc).\n>>>\n>>> I don't have an easy suggestion for making it more atomic, though. You\n>>> could choose one or the other direction using some synchronization\n>>> between the two (e.g., child reassigns only after parent signals over a\n>>> pipe that it has relinquished), but it's all kind of ugly.\n>>>\n>> Ya, that seems really ugly. My first thought was some way to disable\n>> signals temporarily, but I am guessing that either has no good way to do\n>> it or would introduce even more issues. Plus there is always kill -9...\n> \n> I guess the parent could relinquish control by reassigning to \"0\" before\n> even calling fork(), and then taking it back if fork() fails. And then\n> worst case is that nobody cleans up the lock (if the child is killed\n> before taking control), but that's the least-bad outcome from a\n> correctness perspective (though still annoying).\n> \n> -Peff\n\nThis seems simpler than the suggestion of a pipe signal, and the worst\nresult is only the version that is possible no matter what if there is a\nwell timed kill -9.\n"}]}