Re: [PATCH] builtin/fetch: avoid aborting closed reference transaction

[Patrick Steinhardt <ps@pks.im>]

On Thu, Mar 20, 2025 at 07:44:37PM -0500, Justin Tobler wrote:
> As part of the reference transaction commit phase, the transaction is
> set to a closed state regardless of whether it was successful of not.
> Attempting to abort a closed transaction via `ref_transaction_abort()`
> results in a `BUG()`.

Yeah, this is one of the more intricate parts of ref transactions, and
it has been biting me several times in the past. It feels somewhat
similar in spirit to how the `ref_iterator` used to automatically free
itself once it has reached its end, which led to the same class of bugs
due to the interface being way too intricate.

So I wonderer whether we should refactor this interface in the same way:
instead of automatically freeing the transaction on commit/abort, we'd
never do so and require the caller to always free it themselves. This
would make it way easier to use because we can now unconditionally free
the transaction everywhere.

That wouldn't help with the fixed bug though, which is that we call
abort after a failed commit even though the transaction was already
aborted.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 95fd0018b9..f2555731f4 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1867,8 +1867,15 @@ static int do_fetch(struct transport *transport,
>  			goto cleanup;
>  
>  		retcode = ref_transaction_commit(transaction, &err);
> -		if (retcode)
> +		if (retcode) {
> +			/*
> +			 * Explicitly handle transaction cleanup to avoid
> +			 * aborting an already closed transaction.
> +			 */
> +			ref_transaction_free(transaction);
> +			transaction = NULL;
>  			goto cleanup;
> +		}
>  	}
>  
>  	commit_fetch_head(&fetch_head);

Okay, makes sense.

Thanks for the fix!

Patrick