Re: [PATCH v5 2/3] http: avoid concurrent appends to partial packs

[Jeff King <peff@peff.net>]

On Sat, Jul 25, 2026 at 11:44:47PM -0700, Ted Nyman wrote:

> Pack requests stage downloads in a predictable partial-pack file so an
> interrupted transfer can be resumed. Both packfile URI and ordinary dumb
> HTTP requests use this staging path. Opening it in append mode forces
> each write to the current end of the file, so concurrent responses can
> append duplicate data and corrupt the pack.
> 
> Open the partial pack read-write without O_APPEND and seek once to its
> current end. Each downloader then retains the offset matching the Range
> it requested. Because the staging key must uniquely identify immutable
> pack contents, overlapping responses write the same bytes at the same
> offsets instead of extending the file with duplicate data.

OK. I still think this is kind of horrible and gross, but I can't think
of a reason it won't work (at least on POSIX-ish systems) and it solves
the problem with minimal changes and risk of regression.

I wondered about racing with another concurrent writer on the seek, but
I think it is OK. We seek immediately, and then use that offset (which
we get from another seek, replacing ftell()) as the value for our range
request. So even if somebody else advances the file, we have _some_
atomic value that we'll start writing to ourselves, and the worst case
is redundantly requesting a few bytes.

> MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
> existing file. Create a missing partial pack exclusively, close it, and
> reopen it without O_CREAT so every retained descriptor permits another
> downloader to unlink the staging path. Duplicate that descriptor for
> index-pack instead of reopening the path after closing the stream;
> index-pack installs its own pack and the shared staging file is only
> unlinked, never renamed.

This part I have no real knowledge or opinion on the Windows bits (or if
there's an easier way to do it).

> Accept HTTP 416 when a partial pack is already
> complete and let index-pack validate its contents.

I wonder if we still need this or not. AIUI the original 416 responses
came because we were asking for nonsense outside of the range (because
the corrupted writes advanced the file too far). The worst case now is
that we'd ask for bytes "N-" when the file is only N bytes long, and the
server should say "OK, here are your 0 bytes". But maybe there's a
server who complains about that.

> diff --git a/http.c b/http.c
> index caccf2108e..a0d399b274 100644
> --- a/http.c
> +++ b/http.c
> @@ -2688,10 +2688,13 @@ int finish_http_pack_request(struct http_pack_request *preq)
>  	int tmpfile_fd;
>  	int ret = 0;
>  
> +	/* Another downloader may unlink the staging path while we index it. */
> +	tmpfile_fd = xdup(fileno(preq->packfile));
>  	fclose(preq->packfile);
>  	preq->packfile = NULL;
> -
> -	tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
> +	if (lseek(tmpfile_fd, 0, SEEK_SET) < 0)
> +		die_errno("unable to seek local file %s for pack",
> +			  preq->tmpfile.buf);

OK, here we are avoiding the race that it gets unlinked by dup-ing the
existing descriptor and seeking back to the start. Makes sense. But
then...

> @@ -2704,13 +2707,8 @@ int finish_http_pack_request(struct http_pack_request *preq)
>  	else
>  		ip.no_stdout = 1;
>  
> -	if (run_command(&ip)) {
> +	if (run_command(&ip))
>  		ret = -1;
> -		goto cleanup;
> -	}
> -
> -cleanup:
> -	close(tmpfile_fd);
>  	unlink(preq->tmpfile.buf);
>  	return ret;

What is going on with this hunk? We don't really need to jump to cleanup
here because we get there directly anyway, and there are no other users
of the cleanup label. So that part doesn't seem wrong, but rather
unrelated.

More importantly, why don't we need to close tmpfile_fd anymore? We hand
it off to run_command(), which will always close it. So I _think_ it was
always wrong to close it ourselves here. If so, then could this hunk
become a preparatory commit on its own?

This commit is already confusing enough that the more extraneous stuff
we can take out of it the better.

> @@ -2738,22 +2736,45 @@ struct http_pack_request *new_http_pack_request(
> [...]
> +	/*
> +	 * MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
> +	 * existing file; reopen a newly created file so others may unlink it.
> +	 */
> +	for (;;) {
> +		fd = open(preq->tmpfile.buf, O_RDWR);
> +		if (fd >= 0 || errno != ENOENT)
> +			break;
> +		fd = open(preq->tmpfile.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
> +		if (fd >= 0) {
> +			close(fd);
> +			continue;
> +		}
> +		if (errno != EEXIST)
> +			break;
> +	}

OK, and this is the opening magic. What's going on with the O_EXCL here,
though? We try to open once, and if that fails with ENOENT then we open
again. But isn't that racy? Two processes simultaneously try to open(),
find the file is not there, and then both try O_EXCL. Only one of them
will win, and the other will barf.

I guess that is the reason for the loop, where we will try again over
and over until we either pick up somebody else's copy or get our own.
And if we get our own, we still close it and try again. And that's the
Windows magic described in the commit message.

That is...subtle as hell. I really wonder if it would be worth
introducing the basic form of this (just opening once with O_RDWR) and
then doing the Windows hackery on top as a separate commit. That would
leave the intermediate state subject to racy problems on Windows. But
when balancing bisectability versus having a clear human-readable patch,
I think I'd rather see it broken up.

> +	if (fd < 0) {
> +		error_errno("unable to open local file %s for pack",
> +			    preq->tmpfile.buf);
>  		goto abort;
>  	}

OK, and then we get here if we broke out of the loop due to an error
besides ENOENT/EEXIST.

> +	prev_posn = lseek(fd, 0, SEEK_END);
> +	if (prev_posn < 0) {
> +		error_errno("unable to seek local file %s for pack",
> +			    preq->tmpfile.buf);
> +		close(fd);
> +		goto abort;
> +	}
> +	preq->packfile = xfdopen(fd, "w");

And then this is the positioning magic to replace O_APPEND. Good.

> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh

For the record, I don't love that we are using a custom perl script here
instead of going through apache (like all of our other tests). But I
suspect the apache version would be sufficiently horrific (possibly even
worse) that it's not really worth pursuing. Hopefully this perl script
(and the accompanying fifo monstrosities) can sit here for eternity
un-looked-at by human eyes, just quietly doing their job until the heat
death of the universe.

-Peff