Re: [PATCH 3/3] object-file: write ".keep" lockfiles during transaction commit

[Patrick Steinhardt <ps@pks.im>]

On Thu, Oct 15, 2026 at 09:29:01PM -0500, Justin Tobler wrote:
> The "files" ODB backend writes a ".keep" lockfile into the transaction
> quarantine directory via `odb_transaction_files_write_pack()`. When the
> ODB transaction is committed, the ".keep" lockfile is migrated to the
> primary ODB alongside its packfile. Writing the temporary lockfile in
> this manner has a couple of downsides:
> 
>   - Failing to migrate the lockfile during transaction commit due to a
>     concurrent process already acquiring the same lock still allows the
>     rest of the transaction to be migrated into the primary ODB. This
>     dirties the primary ODB with new objects even though no references
>     were updated and the operation failed.

I'm not fully sure I understand this downside. Are you saying that we
fail to migrate the lockfile, then continue migrating everything else
only to then fail the whole transaction?

... skipping ahead a bit...

Oh, I get it now. You propose to first acquire all lockfiles before
migrating anything so that we cannot conflict with a concurrent process
that aims to claim the same locks.

That will of course mean that we will fail the transaction for any
concurrent process that happens to write the same packfiles. But based
on what you claim that was already the case anyway, but now we notice
early and thus won't commit parts of the transaction. Makes sense.

>   - Even more concerning, the ".keep" tempfile atexit(3) handler tries
>     to remove any lockfile of the same name regardless of whether it was
>     created by the same process. If the ".keep" lockfile of the same
>     name for a concurrent process gets deleted prematurely, its packfile
>     becomes at risk of being deleted by concurrent housekeeping.

And this is the other issue that you've already been hinting at.

> diff --git a/object-file.c b/object-file.c
> index ebe46d35de..39e6299be1 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1321,6 +1357,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
>  		delete_tempfile(&temp);
>  		strbuf_release(&temp_path);
>  
> +		if (write_pack_locks(transaction))
> +			return error(_("unable to lock packfiles"));

Okay, we'll now write lockfiles before migrating any of the data. So if
any of the files already happens to exist, we'll bail out here. It would
have been great to maybe also provide a proper failure message via errno
for example, but okay.

> @@ -1477,37 +1510,12 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
>  			return -1;
>  		}
>  
> -		strbuf_addf(&keep_msg, "receive-pack %"PRIuMAX" on %s",
> -			    (uintmax_t)getpid(), hostname);
> -
> -		if (write_pack_lock(transaction->quarantine, pack_name.buf, keep_msg.buf)) {
> -			strbuf_release(&pack_name);
> -			strbuf_release(&keep_msg);
> -			strbuf_addstr(err_msg, "lockfile write failed");
> -			return -1;
> -		}

And here we stop writing lockfiles when writing the pack.

> diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
> index 1b7097179e..cf349eee2c 100755
> --- a/t/t5547-push-quarantine.sh
> +++ b/t/t5547-push-quarantine.sh
> @@ -77,10 +77,9 @@ test_expect_success '.keep file is removed after push' '
>  	git -C keep.git config set receive.unpackLimit 0 &&
>  
>  	# While incoming objects are still quarantined, validate that the
> -	# ".keep" lockfile is present in the quarantine directory.
> +	# ".keep" lockfile is not present in the quarantine directory.
>  	test_hook -C keep.git pre-receive <<-\EOF &&
> -	keep="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.keep)" &&
> -	test -f "$keep"
> +	test ! -e "$GIT_QUARANTINE_PATH"/pack/pack-*.keep
>  	EOF
>  
>  	# After quarantined objects are migrated, validate that the ".keep"
> @@ -101,4 +100,28 @@ test_expect_success '.keep file is removed after push' '
>  	test_path_is_missing "$keep"
>  '
>  
> +test_expect_success '.keep file created outside of process is not removed' '
> +	test_when_finished rm -rf keep.git &&
> +	git init --bare keep.git &&
> +
> +	git -C keep.git config set receive.unpackLimit 0 &&
> +
> +	# Create a ".keep" lockfile matching the packfile to simulate a
> +	# concurrent git-receive-pack(1) that was already written its lockfile.
> +	test_hook -C keep.git pre-receive <<-\EOF &&
> +	pack="$(ls "$GIT_QUARANTINE_PATH"/pack/pack-*.pack)" &&
> +	pack="${pack##*/}" &&
> +	echo "baz" >"objects/pack/${pack%.pack}.keep"
> +	EOF
> +
> +	test_commit bar &&
> +	test_must_fail git push keep.git HEAD &&
> +
> +	# The ".keep" lockfile of the concurrent git-receive-pack(1) should be
> +	# left in its place.
> +	keep="$(ls keep.git/objects/pack/pack-*.keep)" &&
> +	test_path_is_file "$keep" &&
> +	test_grep "baz" "$keep"

Also, there should be no corresponding ".pack" file, right? Do we want
to assert that, too?

Thanks!

Patrick