Volume XXII, number 279Tuesday, October 6, 2026Latest message 33 minutes ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchpack-write, pack-bitmap-write: register tmp pack files for cleanup

3 messages between Sep 25, 2026 and Sep 28, 2026, from Royce Remer, Patrick Steinhardt.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Royce RemerSep 25, 2026, 20:56 UTC on lore

This patch registers the temporary pack files created by git-gc(1) and git-maintenance(1) with the tempfile subsystem so they are removed when the process exits gracefully.

Background ----------

The motivation came from diagnosing disk space in Kubernetes pods running Gitea as git mirrors. If a pod was killed mid-gc, the pack-writing code left orphaned temp files in objects/pack/. On restart, git gc would start fresh and write new temp files alongside the existing ones. Over many restarts these, accumulated until the underlying volume was exhausted:

  Two repositories examined on a single pod:
    objects/pack/tmp_pack_* -- 27 GiB, 21 GiB, 11 GiB, ... (17 files, ~60 GiB total)
    objects/pack/.tmp-*-pack-*.{pack,rev} -- ~118 GiB across 6 killed repacks
  A second pod had a different repository where two killed repacks left:
    objects/pack/.tmp-*-pack-*.{pack,rev} -- ~39 GiB across 2 killed repacks
  Deleting those files and running git-prune-packed(1) to remove loose
  objects already represented in pack files recovered ~224 GiB on that
  second pod alone.

Obviously, this is dependent on repository sizes and number of failures and such, but I thought I'd share my extreme example.

Reviewing the gc and maintenance code, I don't see any attempts to resume or reuse temp files left by a previous invocation; each run calls odb_mkstemp() unconditionally to create a fresh file. Any surviving temp file should be safe to remove.

The tmp_idx, tmp_pack and tmp_bitmap sites predate the tempfile subsystem (1a9d15db25, 2015-08-10) and so had no mechanism to register when introduced. The tmp_rev and tmp_mtimes sites were added afterward but did not use it either.

Note that git-repack(1) already handles this correctly: it calls register_tempfile() for the .tmp-<pid>-pack-<sha>.* files it creates via collect_pack_filenames(), so those are cleaned up on graceful exit. The lower-level paths invoked by git-gc(1) and git-maintenance(1) (pack-write.c and pack-bitmap-write.c) go through odb_mkstemp() which wraps mkstemp(2) directly without registering with the tempfile subsystem, and so do not benefit from this cleanup.

I have some unit tests covering this, but they required instrumenting the code to add a wait driven by an environment variable so I could catch/kill a repack on a tiny mock repo. I decided not to commit those as I think the fix is self-evident and we're just delegating to the same tempfile cleanup logic and relying on that coverage.

Royce Remer (1):
  pack-write, pack-bitmap-write: register tmp pack files for cleanup
 pack-bitmap-write.c | 3 +++
 pack-write.c        | 5 +++++
 2 files changed, 8 insertions(+)
-- 
2.55.0.1.ga30d533ec0
Royce RemerSep 25, 2026, 20:56 UTC in reply to Royce Remer on lore

[PATCH 1/1] pack-write, pack-bitmap-write: register tmp pack files for cleanup

`git repack` correctly uses `register_tempfile()` via `collect_pack_filenames()` for the `.tmp-<pid>-pack-*` files it creates, so they are removed when the process exits gracefully.

The lower-level pack-writing functions invoked by `git gc` and `git maintenance` do not. They call `odb_mkstemp()` which wraps `mkstemp(2)` directly, bypassing the tempfile subsystem entirely. A SIGTERM leaves these files stranded on disk where they accumulate and can exhaust available space:

  objects/pack/tmp_pack_XXXXXX   (create_tmp_packfile)
  objects/pack/tmp_idx_XXXXXX    (write_idx_file)
  objects/pack/tmp_rev_XXXXXX    (write_rev_file_order)
  objects/pack/tmp_mtimes_XXXXXX (write_mtimes_file)
  objects/pack/tmp_bitmap_XXXXXX (bitmap_writer_finish)

Call `register_tempfile()` immediately after each `odb_mkstemp()` so that the atexit(3) and signal handlers unlink the file on abnormal exit.

Signed-off-by: Royce Remer <royceremer@gmail.com>
---
 pack-bitmap-write.c | 3 +++
 pack-write.c        | 5 +++++
 2 files changed, 8 insertions(+)
Show changes to 2 files +8 −0

pack-bitmap-write.c, pack-write.c

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 1bcb3f98a4..c566419690 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -23,6 +23,7 @@
 #include "oid-array.h"
 #include "config.h"
 #include "alloc.h"
+#include "tempfile.h"
 #include "refs.h"
 #include "strmap.h"
 #include "midx.h"
@@ -1378,6 +1379,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
 
 	int fd = odb_mkstemp(writer->repo->objects, &tmp_file,
 			     "pack/tmp_bitmap_XXXXXX");
+	struct tempfile *tmp = register_tempfile(tmp_file.buf);
 
 	if (writer->pseudo_merges_nr)
 		options |= BITMAP_OPT_PSEUDO_MERGES;
@@ -1435,6 +1437,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
 
 	if (rename(tmp_file.buf, filename))
 		die_errno("unable to rename temporary bitmap file to '%s'", filename);
+	delete_tempfile(&tmp);
 
 	strbuf_release(&tmp_file);
 	free(offsets);
diff --git a/pack-write.c b/pack-write.c
index 83eaf88541..fa6b532230 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -13,6 +13,7 @@
 #include "path.h"
 #include "repository.h"
 #include "strbuf.h"
+#include "tempfile.h"
 
 void reset_pack_idx_option(struct pack_idx_option *opts)
 {
@@ -87,6 +88,7 @@ const char *write_idx_file(struct repository *repo,
 			fd = odb_mkstemp(repo->objects, &tmp_file,
 					 "pack/tmp_idx_XXXXXX");
 			index_name = strbuf_detach(&tmp_file, NULL);
+			(void)register_tempfile(index_name);
 		} else {
 			unlink(index_name);
 			fd = xopen(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
@@ -263,6 +265,7 @@ char *write_rev_file_order(struct repository *repo,
 			fd = odb_mkstemp(repo->objects, &tmp_file,
 					 "pack/tmp_rev_XXXXXX");
 			path = strbuf_detach(&tmp_file, NULL);
+			(void)register_tempfile(path);
 		} else {
 			unlink(rev_name);
 			fd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
@@ -346,6 +349,7 @@ static char *write_mtimes_file(struct repository *repo,
 
 	fd = odb_mkstemp(repo->objects, &tmp_file, "pack/tmp_mtimes_XXXXXX");
 	mtimes_name = strbuf_detach(&tmp_file, NULL);
+	(void)register_tempfile(mtimes_name);
 	f = hashfd(repo->hash_algo, fd, mtimes_name);
 
 	write_mtimes_header(repo->hash_algo, f);
@@ -535,6 +539,7 @@ struct hashfile *create_tmp_packfile(struct repository *repo,
 
 	fd = odb_mkstemp(repo->objects, &tmpname, "pack/tmp_pack_XXXXXX");
 	*pack_tmp_name = strbuf_detach(&tmpname, NULL);
+	(void)register_tempfile(*pack_tmp_name);
 	return hashfd(repo->hash_algo, fd, *pack_tmp_name);
 }
 
-- 
2.55.0.1.ga30d533ec0
Patrick SteinhardtSep 28, 2026, 07:58 UTC in reply to Royce Remer on lore

Re: [PATCH 1/1] pack-write, pack-bitmap-write: register tmp pack files for cleanup

On Fri, Sep 25, 2026 at 01:56:33PM -0700, Royce Remer wrote:
Show 20 quoted lines
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 1bcb3f98a4..c566419690 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -1378,6 +1379,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
>  
>  	int fd = odb_mkstemp(writer->repo->objects, &tmp_file,
>  			     "pack/tmp_bitmap_XXXXXX");
> +	struct tempfile *tmp = register_tempfile(tmp_file.buf);
>  
>  	if (writer->pseudo_merges_nr)
>  		options |= BITMAP_OPT_PSEUDO_MERGES;
> @@ -1435,6 +1437,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
>  
>  	if (rename(tmp_file.buf, filename))
>  		die_errno("unable to rename temporary bitmap file to '%s'", filename);
> +	delete_tempfile(&tmp);
>  
>  	strbuf_release(&tmp_file);
>  	free(offsets);

The fact that we add calls to `register_tempfile()` to almost every single callsites that uses `odb_mkstemp()` makes me wonder whether the interface itself is maybe misdesigned. Like, should it maybe return a tempfile instead of returning a file descriptor so that callers don't have to manually register it?

I also wonder whether `odb_mkstemp()` even sits at the right level to begin with. It's ultimately specific to the "files" backend, as it assumes that files live in "objects/". Would it be preferable if we instead made it part of the "tempfile.h" API, where the only difference to other functions is that it knows to also support leading directories?

We could for example have a new "_d" suffix for `mks_tempfile()` functions.

Thanks!
Patrick

Back to recent threads