[PATCH v2 0/5] repack tempfile-cleanup signal deadlock

On Fri, Oct 21, 2022 at 05:41:02PM -0400, Jeff King wrote:

And here's a re-roll based on feedback on v1. Poor Jan; I hope this thread isn't blowing up your inbox. :) But thank you for reporting the problem!

The changes from v1 are:

  - clarified rationale in patch 2
  - new patch 3 improves an error message and reduces scope of fname_old
  - patch 4 cleans up fname_old to use tempfile path

Range diff is below.

  [1/5]: repack: convert "names" util bitfield to array
  [2/5]: repack: populate extension bits incrementally
  [3/5]: repack: expand error message for missing pack files
  [4/5]: repack: use tempfiles for signal cleanup
  [5/5]: repack: drop remove_temporary_files()

 builtin/repack.c  | 90 +++++++++++++++--------------------------------
 t/t7700-repack.sh | 16 +++++++++
 2 files changed, 45 insertions(+), 61 deletions(-)

1:  6de9bd9d71 = 1:  affb21442f repack: convert "names" util bitfield to array
2:  7d72ed9a39 ! 2:  18d8318881 repack: populate extension bits incrementally
    @@ Commit message
         iterate over the "names" list (which contains hashes of packs generated
         by pack-objects), and call populate_pack_exts() for each.
     
    -    There are two small problems with this:
    +    There's one small problem with this. In repack_promisor_objects(), we
    +    may add entries to "names" and call populate_pack_exts() for them.
    +    Calling it again is mostly just wasteful, as we'll stat() the filename
    +    with each possible extension, get the same result, and just overwrite
    +    our bits.
     
    -      - repack_promisor_objects() may have added entries to "names", and
    -        already called populate_pack_exts() for them. This is mostly just
    -        wasteful, as we'll stat() the filename with each possible extension,
    -        get the same result, and just overwrite our bits. But it makes the
    -        code flow confusing, and it will become a problem if we try to make
    -        populate_pack_exts() do more things.
    +    So we could drop the call there, and leave the final loop to populate
    +    all of the bits. But instead, this patch does the reverse: drops the
    +    final loop, and teaches the other two sites to populate the bits as they
    +    add entries.
     
    -      - it would be nice to record the generated filenames as soon as
    -        possible. We don't currently use them for cleaning up from a failed
    -        operation, but a future patch will do so.
    +    This makes the code easier to reason about, as you never have to worry
    +    about when the util field is valid; it is always valid for each entry.
    +
    +    It also serves my ulterior purpose: recording the generated filenames as
    +    soon as possible will make it easier for a future patch to use them for
    +    cleaning up from a failed operation.
     
         Signed-off-by: Jeff King <peff@peff.net>
     
-:  ---------- > 3:  bbd7f82f88 repack: expand error message for missing pack files
3:  16448019ba ! 4:  7cd89d1575 repack: use tempfiles for signal cleanup
    @@ Commit message
         pack. Before this patch, the .tmp-* file for the main pack would have
         been left, but now we correctly clean it up.
     
    +    Two small subtleties on the implementation:
    +
    +      - in the renaming loop, we can stop re-constructing fname_old; we only
    +        use it when we have a tempfile to rename, so we can just ask the
    +        tempfile for its path (which, barring bugs, should be identical)
    +
    +      - when renaming fails, our error message mentions fname_old. But since
    +        a failed rename_tempfile() invalidates the tempfile struct, we'll
    +        lose access to that string. Instead, let's mention the destination
    +        filename, which is what most other callers do.
    +
         Reported-by: Jan Pokorný <poki@fnusa.cz>
         Signed-off-by: Jeff King <peff@peff.net>
     
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
      
      	show_progress = !po_args.quiet && isatty(2);
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
    - 			fname_old = mkpathdup("%s-%s%s",
    - 					packtmp, item->string, exts[ext].name);
    + 		struct generated_pack_data *data = item->util;
    + 
    + 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
    +-			char *fname, *fname_old;
    ++			char *fname;
    + 
    + 			fname = mkpathdup("%s/pack-%s%s",
    + 					packdir, item->string, exts[ext].name);
    +-			fname_old = mkpathdup("%s-%s%s",
    +-					packtmp, item->string, exts[ext].name);
      
     -			if (data->exts[ext]) {
     +			if (data->tempfiles[ext]) {
    ++				const char *fname_old = get_tempfile_path(data->tempfiles[ext]);
      				struct stat statbuffer;
    ++
      				if (!stat(fname_old, &statbuffer)) {
      					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
      					chmod(fname_old, statbuffer.st_mode);
      				}
      
     -				if (rename(fname_old, fname))
    +-					die_errno(_("renaming '%s' failed"), fname_old);
     +				if (rename_tempfile(&data->tempfiles[ext], fname))
    - 					die_errno(_("renaming '%s' failed"), fname_old);
    ++					die_errno(_("renaming pack to '%s' failed"), fname);
      			} else if (!exts[ext].optional)
    - 				die(_("missing required file: %s"), fname_old);
    + 				die(_("pack-objects did not write a '%s' file for pack %s-%s"),
    + 				    exts[ext].name, packtmp, item->string);
    + 			else if (unlink(fname) < 0 && errno != ENOENT)
    + 				die_errno(_("could not unlink: %s"), fname);
    + 
    + 			free(fname);
    +-			free(fname_old);
    + 		}
    + 	}
    + 	/* End of pack replacement. */
     
      ## t/t7700-repack.sh ##
     @@ t/t7700-repack.sh: test_expect_success TTY '--quiet disables progress' '
4:  c7b0f3823d = 5:  d7e9225b8e repack: drop remove_temporary_files()