{"thread":{"id":"66394","subject":"[PATCH 0/1] pack-write, pack-bitmap-write: register tmp pack files for cleanup","startedAt":"2026-09-25T20:57:05Z","lastAt":"2026-09-28T07:58:47Z","messageCount":3,"participants":["Royce Remer","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"553330","messageId":"20260925205633.530651-1-royceremer@gmail.com","threadId":"66394","inReplyTo":null,"subject":"[PATCH 0/1] pack-write, pack-bitmap-write: register tmp pack files for cleanup","fromName":"Royce Remer","fromEmail":"royceremer@gmail.com","sentAt":"2026-09-25T20:56:32Z","receivedAt":"2026-09-25T20:57:05Z","isPatch":true,"body":"This patch registers the temporary pack files created by git-gc(1) and\ngit-maintenance(1) with the tempfile subsystem so they are removed when\nthe process exits gracefully.\n\nBackground\n----------\n\nThe motivation came from diagnosing disk space in Kubernetes pods\nrunning Gitea as git mirrors.  If a pod was killed mid-gc, the\npack-writing code left orphaned temp files in objects/pack/.\nOn restart, git gc would start fresh and write new temp files\nalongside the existing ones.  Over many restarts these,\naccumulated until the underlying volume was exhausted:\n\n  Two repositories examined on a single pod:\n    objects/pack/tmp_pack_* -- 27 GiB, 21 GiB, 11 GiB, ... (17 files, ~60 GiB total)\n    objects/pack/.tmp-*-pack-*.{pack,rev} -- ~118 GiB across 6 killed repacks\n\n  A second pod had a different repository where two killed repacks left:\n    objects/pack/.tmp-*-pack-*.{pack,rev} -- ~39 GiB across 2 killed repacks\n\n  Deleting those files and running git-prune-packed(1) to remove loose\n  objects already represented in pack files recovered ~224 GiB on that\n  second pod alone.\n\nObviously, this is dependent on repository sizes and number of\nfailures and such, but I thought I'd share my extreme example.\n\nReviewing the gc and maintenance code, I don't see any attempts to\nresume or reuse temp files left by a previous invocation; each run\ncalls odb_mkstemp() unconditionally to create a fresh file.  Any\nsurviving temp file should be safe to remove.\n\nThe tmp_idx, tmp_pack and tmp_bitmap sites predate the tempfile\nsubsystem (1a9d15db25, 2015-08-10) and so had no mechanism to\nregister when introduced.  The tmp_rev and tmp_mtimes sites\nwere added afterward but did not use it either.\n\nNote that git-repack(1) already handles this correctly: it calls\nregister_tempfile() for the .tmp-<pid>-pack-<sha>.* files it creates\nvia collect_pack_filenames(), so those are cleaned up on graceful exit.\nThe lower-level paths invoked by git-gc(1) and git-maintenance(1)\n(pack-write.c and pack-bitmap-write.c) go through odb_mkstemp() which\nwraps mkstemp(2) directly without registering with the tempfile\nsubsystem, and so do not benefit from this cleanup.\n\nI have some unit tests covering this, but they required instrumenting\nthe code to add a wait driven by an environment variable so I could\ncatch/kill a repack on a tiny mock repo. I decided not to commit\nthose as I think the fix is self-evident and we're just delegating\nto the same tempfile cleanup logic and relying on that coverage.\n\nRoyce Remer (1):\n  pack-write, pack-bitmap-write: register tmp pack files for cleanup\n\n pack-bitmap-write.c | 3 +++\n pack-write.c        | 5 +++++\n 2 files changed, 8 insertions(+)\n\n-- \n2.55.0.1.ga30d533ec0\n\n"},{"id":"553331","messageId":"20260925205633.530651-2-royceremer@gmail.com","threadId":"66394","inReplyTo":"20260925205633.530651-1-royceremer@gmail.com","subject":"[PATCH 1/1] pack-write, pack-bitmap-write: register tmp pack files for cleanup","fromName":"Royce Remer","fromEmail":"royceremer@gmail.com","sentAt":"2026-09-25T20:56:33Z","receivedAt":"2026-09-25T20:57:25Z","isPatch":true,"body":"`git repack` correctly uses `register_tempfile()` via\n`collect_pack_filenames()` for the `.tmp-<pid>-pack-*` files it\ncreates, so they are removed when the process exits gracefully.\n\nThe lower-level pack-writing functions invoked by `git gc` and\n`git maintenance` do not. They call `odb_mkstemp()` which wraps\n`mkstemp(2)` directly, bypassing the tempfile subsystem entirely.\nA SIGTERM leaves these files stranded on disk where they accumulate\nand can exhaust available space:\n\n  objects/pack/tmp_pack_XXXXXX   (create_tmp_packfile)\n  objects/pack/tmp_idx_XXXXXX    (write_idx_file)\n  objects/pack/tmp_rev_XXXXXX    (write_rev_file_order)\n  objects/pack/tmp_mtimes_XXXXXX (write_mtimes_file)\n  objects/pack/tmp_bitmap_XXXXXX (bitmap_writer_finish)\n\nCall `register_tempfile()` immediately after each `odb_mkstemp()` so\nthat the atexit(3) and signal handlers unlink the file on abnormal\nexit.\n\nSigned-off-by: Royce Remer <royceremer@gmail.com>\n---\n pack-bitmap-write.c | 3 +++\n pack-write.c        | 5 +++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 1bcb3f98a4..c566419690 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -23,6 +23,7 @@\n #include \"oid-array.h\"\n #include \"config.h\"\n #include \"alloc.h\"\n+#include \"tempfile.h\"\n #include \"refs.h\"\n #include \"strmap.h\"\n #include \"midx.h\"\n@@ -1378,6 +1379,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,\n \n \tint fd = odb_mkstemp(writer->repo->objects, &tmp_file,\n \t\t\t     \"pack/tmp_bitmap_XXXXXX\");\n+\tstruct tempfile *tmp = register_tempfile(tmp_file.buf);\n \n \tif (writer->pseudo_merges_nr)\n \t\toptions |= BITMAP_OPT_PSEUDO_MERGES;\n@@ -1435,6 +1437,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,\n \n \tif (rename(tmp_file.buf, filename))\n \t\tdie_errno(\"unable to rename temporary bitmap file to '%s'\", filename);\n+\tdelete_tempfile(&tmp);\n \n \tstrbuf_release(&tmp_file);\n \tfree(offsets);\ndiff --git a/pack-write.c b/pack-write.c\nindex 83eaf88541..fa6b532230 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -13,6 +13,7 @@\n #include \"path.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"tempfile.h\"\n \n void reset_pack_idx_option(struct pack_idx_option *opts)\n {\n@@ -87,6 +88,7 @@ const char *write_idx_file(struct repository *repo,\n \t\t\tfd = odb_mkstemp(repo->objects, &tmp_file,\n \t\t\t\t\t \"pack/tmp_idx_XXXXXX\");\n \t\t\tindex_name = strbuf_detach(&tmp_file, NULL);\n+\t\t\t(void)register_tempfile(index_name);\n \t\t} else {\n \t\t\tunlink(index_name);\n \t\t\tfd = xopen(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n@@ -263,6 +265,7 @@ char *write_rev_file_order(struct repository *repo,\n \t\t\tfd = odb_mkstemp(repo->objects, &tmp_file,\n \t\t\t\t\t \"pack/tmp_rev_XXXXXX\");\n \t\t\tpath = strbuf_detach(&tmp_file, NULL);\n+\t\t\t(void)register_tempfile(path);\n \t\t} else {\n \t\t\tunlink(rev_name);\n \t\t\tfd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n@@ -346,6 +349,7 @@ static char *write_mtimes_file(struct repository *repo,\n \n \tfd = odb_mkstemp(repo->objects, &tmp_file, \"pack/tmp_mtimes_XXXXXX\");\n \tmtimes_name = strbuf_detach(&tmp_file, NULL);\n+\t(void)register_tempfile(mtimes_name);\n \tf = hashfd(repo->hash_algo, fd, mtimes_name);\n \n \twrite_mtimes_header(repo->hash_algo, f);\n@@ -535,6 +539,7 @@ struct hashfile *create_tmp_packfile(struct repository *repo,\n \n \tfd = odb_mkstemp(repo->objects, &tmpname, \"pack/tmp_pack_XXXXXX\");\n \t*pack_tmp_name = strbuf_detach(&tmpname, NULL);\n+\t(void)register_tempfile(*pack_tmp_name);\n \treturn hashfd(repo->hash_algo, fd, *pack_tmp_name);\n }\n \n-- \n2.55.0.1.ga30d533ec0\n\n"},{"id":"553419","messageId":"aroeMVWrwvlC1MrH@pks.im","threadId":"66394","inReplyTo":"20260925205633.530651-2-royceremer@gmail.com","subject":"Re: [PATCH 1/1] pack-write, pack-bitmap-write: register tmp pack files for cleanup","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-28T07:58:41Z","receivedAt":"2026-09-28T07:58:47Z","isPatch":true,"body":"On Fri, Sep 25, 2026 at 01:56:33PM -0700, Royce Remer wrote:\n> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\n> index 1bcb3f98a4..c566419690 100644\n> --- a/pack-bitmap-write.c\n> +++ b/pack-bitmap-write.c\n> @@ -1378,6 +1379,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,\n>  \n>  \tint fd = odb_mkstemp(writer->repo->objects, &tmp_file,\n>  \t\t\t     \"pack/tmp_bitmap_XXXXXX\");\n> +\tstruct tempfile *tmp = register_tempfile(tmp_file.buf);\n>  \n>  \tif (writer->pseudo_merges_nr)\n>  \t\toptions |= BITMAP_OPT_PSEUDO_MERGES;\n> @@ -1435,6 +1437,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,\n>  \n>  \tif (rename(tmp_file.buf, filename))\n>  \t\tdie_errno(\"unable to rename temporary bitmap file to '%s'\", filename);\n> +\tdelete_tempfile(&tmp);\n>  \n>  \tstrbuf_release(&tmp_file);\n>  \tfree(offsets);\n\nThe fact that we add calls to `register_tempfile()` to almost every\nsingle callsites that uses `odb_mkstemp()` makes me wonder whether the\ninterface itself is maybe misdesigned. Like, should it maybe return a\ntempfile instead of returning a file descriptor so that callers don't\nhave to manually register it?\n\nI also wonder whether `odb_mkstemp()` even sits at the right level to\nbegin with. It's ultimately specific to the \"files\" backend, as it\nassumes that files live in \"objects/\". Would it be preferable if we\ninstead made it part of the \"tempfile.h\" API, where the only difference\nto other functions is that it knows to also support leading directories?\n\nWe could for example have a new \"_d\" suffix for `mks_tempfile()`\nfunctions.\n\nThanks!\n\nPatrick\n"}]}