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

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.

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...

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.

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.

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