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

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 24, 2025, 10:40 UTC
Message-ID: <Z-E2ns9VziejSLB2@pks.im>
In-Reply-To: <20250321004437.505461-1-jltobler@gmail.com>

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.

Show 21 quoted lines

> 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