Re: [PATCH v4 00/12] builtin/maintenance: fix ref lock races when detaching

> 
> Le 3 juin 2025 à 10:01, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> Hi,
> 
> this patch series fixes races around locking the "packed-refs" file when
> auto-maintenance decides to repack it. This issue has been reported e.g.
> via [1] and [2].
> 
> The root cause is that git-gc(1) used to know to detach _after_ having
> repacked references. As such, callers wouldn't continue with their thing
> until we have already packed refs, and thus the race does not exist
> there. git-maintenance(1) didn't have the same split though, so this
> patch series retrofits that logic.
> 
> The series is structured as follows:
> 
>  - Patches 1 and 2 do some light refactorings.
> 
>  - Patches 3 to 5 refactor how we set up the list of tasks to not rely
>    on globals anymore. Instead, we now have a single source of truth
>    for which tasks exactly will be run.
> 
>  - The remaining patches introduce the split of before/after-detach
>    tasks and wire them up for "pack-refs", "reflog-expire" and "gc"
>    tasks.
> 
> Changes in v2:
>  - A couple of commit message improvements.
>  - Introduce `die(NULL)` to die with the correct exit code but no error
>    message. This gets rid of some magic numbers.
>  - Introduce an enum to discern the phases before and after detach.
>  - Link to v1: https://lore.kernel.org/r/20250527-b4-pks-maintenance-ref-lock-race-v1-0-e1ceb2dea66e@pks.im
> 
> Changes in v3:
>  - Rework logic to talk about foreground/background tasks instead of
>    before/after detach.
>  - Link to v2: https://lore.kernel.org/r/20250530-b4-pks-maintenance-ref-lock-race-v2-0-d04e2f93e51f@pks.im
> 
> Changes in v4:
>  - Some more massaging of commit messages.
>  - Link to v3: https://lore.kernel.org/r/20250602-b4-pks-maintenance-ref-lock-race-v3-0-587d44252dcb@pks.im
> 
> Thanks!
> 
> Patrick
> 
> [1]: <CAJR-fbZ4X1+gN75m2dUvocR6NkowLOZ9F26cjBy8w1qd181OoQ@mail.gmail.com>
> [2]: <CANi7bVAkNc+gY1NoXfJuDRjxjZLTgL8Lfn8_ZmWsvLAoiLPkNg@mail.gmail.com>
> 
> ---
> Patrick Steinhardt (12):
>      builtin/gc: use designated field initializers for maintenance tasks
>      builtin/gc: drop redundant local variable
>      builtin/maintenance: centralize configuration of explicit tasks
>      builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
>      builtin/maintenance: stop modifying global array of tasks
>      builtin/maintenance: extract function to run tasks
>      builtin/maintenance: fix typedef for function pointers
>      builtin/maintenance: split into foreground and background tasks
>      builtin/maintenance: fix locking race with refs and reflogs tasks
>      usage: allow dying without writing an error message
>      builtin/gc: avoid global state in `gc_before_repack()`
>      builtin/maintenance: fix locking race when handling "gc" task
> 
> builtin/am.c                |   4 +-
> builtin/checkout.c          |   4 +-
> builtin/fetch.c             |   2 +-
> builtin/gc.c                | 410 +++++++++++++++++++++++++-------------------
> builtin/submodule--helper.c |  12 +-
> t/t7900-maintenance.sh      |  19 +-
> usage.c                     |   2 +
> 7 files changed, 263 insertions(+), 190 deletions(-)
> 
> Range-diff versus v3:
> 
> 1:  e46a65951b9 =  1:  280f13d2895 builtin/gc: use designated field initializers for maintenance tasks
> 2:  73cd67f3e1a =  2:  16a017fb819 builtin/gc: drop redundant local variable
> 3:  a02452a6d6f =  3:  0ab3344ddb0 builtin/maintenance: centralize configuration of explicit tasks
> 4:  ccd7691e4d5 =  4:  69e768cb54e builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
> 5:  0e243fd81e6 =  5:  295e9e5ee9f builtin/maintenance: stop modifying global array of tasks
> 6:  c95bd62823e =  6:  d94b0c86622 builtin/maintenance: extract function to run tasks
> 7:  43d28434d8e =  7:  0bbba671cd0 builtin/maintenance: fix typedef for function pointers
> 8:  d5740a5c9d9 =  8:  4ce38539bb6 builtin/maintenance: split into foreground and background tasks
> 9:  168eb3a9372 !  9:  28092b9bed1 builtin/maintenance: fix locking race when packing refs and reflogs
>    @@ Metadata
>     Author: Patrick Steinhardt <ps@pks.im>
> 
>      ## Commit message ##
>    -    builtin/maintenance: fix locking race when packing refs and reflogs
>    +    builtin/maintenance: fix locking race with refs and reflogs tasks
> 
>         As explained in the preceding commit, git-gc(1) knows to detach only
>    -    after it has already packed references and reflogs. This is done to
>    -    avoid racing around their respective lockfiles.
>    +    after it has already packed references and expired reflogs. This is done
>    +    to avoid racing around their respective lockfiles.
> 
>         Adapt git-maintenance(1) accordingly and run the "pack-refs" and
>         "reflog-expire" tasks in the foreground. Note that the "gc" task has the
> 10:  0ff01f6e2aa ! 10:  b8ed080c67d usage: allow dying without writing an error message
>    @@ Commit message
>         usage: allow dying without writing an error message
> 
>         Sometimes code wants to die in a situation where it already has written
>    -    an error message. To use the same error code as `die()` we have to open
>    -    code the code with a call to `exit(128)` in such cases, which is easy to
>    -    get wrong and leaves magical numbers all over our codebase.
>    +    an error message. To use the same error code as `die()` we have to use
>    +    `exit(128)`, which is easy to get wrong and leaves magic numbers all
>    +    over our codebase.
> 
>         Teach `die_message_builtin()` to not print any error when passed a
>         `NULL` pointer as error string. Like this, such users can now call
> 11:  93f53000e47 = 11:  5b149886263 builtin/gc: avoid global state in `gc_before_repack()`
> 12:  01095d1bf88 = 12:  9ba01f143b3 builtin/maintenance: fix locking race when handling "gc" task
> 
> ---
> base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
> change-id: 20250527-b4-pks-maintenance-ref-lock-race-11ae5d68e06f