http: avoid concurrent appends to partial packs
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. Duplicate the staging descriptor for index-pack instead of reopening the path after closing the stream. Another downloader may unlink the staging path before indexing begins, but index-pack can still read the retained descriptor. Exercise resumed transfers and overlapping 200 and 206 responses, and clarify the staging-key documentation. Signed-off-by: Ted Nyman <tnyman@openai.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ted Nyman committed
Jul 26, 2026 at 17:28 UTC
5e855d9b426dd01552ecbfb47062b90fe53b3df5
3 files changed
+187
-16
Documentation/git-http-fetch.adoc
+3
-2
@@ -48,8 +48,9 @@ commit-id::
48
line (which is not expected in
49
this case), 'git http-fetch' fetches the packfile directly at the given
50
URL and uses index-pack to generate corresponding .idx and .keep files.
51
- The hash is used to determine the name of the temporary file and is
52
- arbitrary. The output of index-pack is printed to stdout. Requires
51
+ The hash is used to determine the name of the temporary file. It need
52
+ not be the pack hash, but it must uniquely identify the pack contents
53
+ for resumption. The output of index-pack is printed to stdout. Requires
54
one or more --index-pack-arg options.
55
56
--index-pack-arg=<arg>::
http.c
+20
-14
@@ -2688,10 +2688,13 @@ int finish_http_pack_request(struct http_pack_request *preq)
2688
int tmpfile_fd;
2689
int ret = 0;
2690
2691
+ /* Another downloader may unlink the staging path while we index it. */
2692
+ tmpfile_fd = xdup(fileno(preq->packfile));
2693
fclose(preq->packfile);
2694
preq->packfile = NULL;
2693
-
2694
- tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
2695
+ if (lseek(tmpfile_fd, 0, SEEK_SET) < 0)
2696
+ die_errno("unable to seek local file %s for pack",
2697
+ preq->tmpfile.buf);
2698
2699
ip.git_cmd = 1;
2700
ip.in = tmpfile_fd;
@@ -2733,22 +2736,30 @@ struct http_pack_request *new_http_pack_request(
2736
struct http_pack_request *new_direct_http_pack_request(
2737
const unsigned char *packed_git_hash, char *url)
2738
{
2736
- off_t prev_posn = 0;
2739
+ off_t prev_posn;
2740
struct http_pack_request *preq;
2741
+ int fd;
2742
2743
CALLOC_ARRAY(preq, 1);
2744
strbuf_init(&preq->tmpfile, 0);
2741
-
2745
preq->url = url;
2746
2747
odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
2748
strbuf_addstr(&preq->tmpfile, ".temp");
2746
- preq->packfile = fopen(preq->tmpfile.buf, "a");
2747
- if (!preq->packfile) {
2748
- error("Unable to open local file %s for pack",
2749
- preq->tmpfile.buf);
2749
+ fd = open(preq->tmpfile.buf, O_RDWR | O_CREAT, 0666);
2750
+ if (fd < 0) {
2751
+ error_errno("unable to open local file %s for pack",
2752
+ preq->tmpfile.buf);
2753
+ goto abort;
2754
+ }
2755
+ prev_posn = lseek(fd, 0, SEEK_END);
2756
+ if (prev_posn < 0) {
2757
+ error_errno("unable to seek local file %s for pack",
2758
+ preq->tmpfile.buf);
2759
+ close(fd);
2760
goto abort;
2761
}
2762
+ preq->packfile = xfdopen(fd, "w");
2763
2764
preq->slot = get_active_slot();
2765
preq->headers = object_request_headers();
@@ -2757,12 +2768,7 @@ struct http_pack_request *new_direct_http_pack_request(
2768
curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
2769
curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER, preq->headers);
2770
2760
- /*
2761
- * If there is data present from a previous transfer attempt,
2762
- * resume where it left off
2763
- */
2764
- prev_posn = ftello(preq->packfile);
2765
- if (prev_posn>0) {
2771
+ if (prev_posn > 0) {
2772
if (http_is_verbose)
2773
fprintf(stderr,
2774
"Resuming fetch of pack %s at byte %"PRIuMAX"\n",
t/t5550-http-fetch-dumb.sh
+164
@@ -312,6 +312,170 @@ test_expect_success 'http-fetch --packfile accepts an already complete partial'
312
git -C packfileclient-complete cat-file -e "$HASH"
313
'
314
315
+test_expect_success 'http-fetch --packfile resumes a partial download' '
316
+ git init packfileclient-resume &&
317
+ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
318
+ ls objects/pack/pack-*.pack) &&
319
+ tmpfile="packfileclient-resume/.git/objects/pack/pack-$ARBITRARY.pack.temp" &&
320
+ test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" &&
321
+ GIT_TRACE_CURL="$TRASH_DIRECTORY/resume.trace" \
322
+ git -C packfileclient-resume http-fetch --packfile="$ARBITRARY" \
323
+ --index-pack-arg=index-pack --index-pack-arg=--stdin \
324
+ --index-pack-arg=--keep \
325
+ "$HTTPD_URL/dumb/repo_pack.git/$p" >out &&
326
+ test_grep "Range: bytes=64-" resume.trace &&
327
+ test_path_is_missing "$tmpfile" &&
328
+ git -C packfileclient-resume cat-file -e "$HASH"
329
+'
330
+
331
+test_expect_success PERL,PIPE 'concurrent http-fetch --packfile cannot corrupt an overlapping download' '
332
+ git init packfileclient-overlap &&
333
+ blob=$(test-tool genrandom pack-overlap 2m |
334
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \
335
+ hash-object -w --stdin) &&
336
+ packhash=$(printf "%s\n" "$blob" |
337
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \
338
+ pack-objects "$TRASH_DIRECTORY/overlap-pack") &&
339
+ pack="$TRASH_DIRECTORY/overlap-pack-$packhash.pack" &&
340
+ tmpfile="packfileclient-overlap/.git/objects/pack/pack-$packhash.pack.temp" &&
341
+ mkfifo server-ready first-ready &&
342
+ exec 7<>server-ready &&
343
+ exec 8<>first-ready &&
344
+ write_script slow-pack-server "$PERL_PATH" <<-\EOF &&
345
+ use strict;
346
+ use warnings;
347
+ use IO::Socket::INET;
348
+
349
+ my ($packfile, $server_ready, $first_ready) = @ARGV;
350
+ my $completed = 0;
351
+ END {
352
+ if (!$completed) {
353
+ signal_ready($server_ready, "failed");
354
+ signal_ready($first_ready, "failed");
355
+ }
356
+ }
357
+
358
+ $SIG{ALRM} = sub { die "timed out serving concurrent pack requests\n" };
359
+ alarm 60;
360
+
361
+ open(my $in, "<:raw", $packfile) or die "open $packfile: $!";
362
+ my $pack = do { local $/; <$in> };
363
+ close($in) or die "close $packfile: $!";
364
+ my $server = IO::Socket::INET->new(LocalAddr => "127.0.0.1",
365
+ LocalPort => 0, Proto => "tcp", Listen => 2, ReuseAddr => 1)
366
+ or die "listen: $!";
367
+
368
+ sub signal_ready {
369
+ my ($file, $value) = @_;
370
+ open(my $out, ">", $file) or die "open $file: $!";
371
+ print $out "$value\n" or die "write $file: $!";
372
+ close($out) or die "close $file: $!";
373
+ }
374
+
375
+ sub write_all {
376
+ my ($out, $data) = @_;
377
+ my $offset = 0;
378
+ while ($offset < length($data)) {
379
+ my $written = syswrite($out, $data,
380
+ length($data) - $offset, $offset);
381
+ defined($written) && $written or die "write response: $!";
382
+ $offset += $written;
383
+ }
384
+ }
385
+
386
+ sub start_response {
387
+ my $out = $server->accept() or die "accept: $!";
388
+ <$out> or die "read request: $!";
389
+ my $start = 0;
390
+ while (<$out>) {
391
+ last if /^\r?\n$/;
392
+ $start = $1 if /^Range: bytes=(\d+)-/i;
393
+ }
394
+ $start < length($pack) or die "invalid range $start";
395
+ my $length = length($pack) - $start;
396
+ my $middle = int($length / 2);
397
+ my $status = $start ? "206 Partial Content" : "200 OK";
398
+ my $headers = "HTTP/1.1 $status\r\n" .
399
+ "Content-Length: $length\r\n" .
400
+ ($start ? "Content-Range: bytes $start-" .
401
+ (length($pack) - 1) . "/" . length($pack) . "\r\n" : "") .
402
+ "Connection: close\r\n\r\n";
403
+ write_all($out, $headers);
404
+ write_all($out, substr($pack, $start, $middle));
405
+ return ($out, $start + $middle);
406
+ }
407
+
408
+ signal_ready($server_ready, $server->sockport());
409
+ my ($first, $first_pos) = start_response();
410
+ signal_ready($first_ready, "ready");
411
+ my ($second, $second_pos) = start_response();
412
+ write_all($first, substr($pack, $first_pos));
413
+ write_all($second, substr($pack, $second_pos));
414
+ close($first) or die "close first response: $!";
415
+ close($second) or die "close second response: $!";
416
+ $completed = 1;
417
+ alarm 0;
418
+ EOF
419
+ {
420
+ "$TRASH_DIRECTORY/slow-pack-server" "$pack" \
421
+ "$TRASH_DIRECTORY/server-ready" \
422
+ "$TRASH_DIRECTORY/first-ready" >server.log 2>&1 &
423
+ server_pid=$!
424
+ } &&
425
+ test_when_finished "
426
+ kill $server_pid 2>/dev/null || :
427
+ wait $server_pid 2>/dev/null || :
428
+ exec 7>&-
429
+ exec 8>&-
430
+ rm -f server-ready first-ready slow-pack-server
431
+ " &&
432
+ read port <&7 &&
433
+ url="http://127.0.0.1:$port/pack" &&
434
+ {
435
+ (
436
+ if ! GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-first.trace" \
437
+ GIT_TRACE_CURL_NO_DATA=1 \
438
+ git -C packfileclient-overlap http-fetch --packfile="$packhash" \
439
+ --index-pack-arg=index-pack \
440
+ --index-pack-arg=--stdin --index-pack-arg=--keep \
441
+ "$url" >first.out
442
+ then
443
+ echo failed >"$TRASH_DIRECTORY/first-ready" &&
444
+ exit 1
445
+ fi
446
+ ) &
447
+ first_pid=$!
448
+ } &&
449
+ test_when_finished "
450
+ kill $first_pid 2>/dev/null || :
451
+ wait $first_pid 2>/dev/null || :
452
+ " &&
453
+ read ready <&8 &&
454
+ test "$ready" = ready &&
455
+ test_path_is_file "$tmpfile" &&
456
+ {
457
+ GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-second.trace" \
458
+ GIT_TRACE_CURL_NO_DATA=1 \
459
+ git -C packfileclient-overlap http-fetch --packfile="$packhash" \
460
+ --index-pack-arg=index-pack \
461
+ --index-pack-arg=--stdin --index-pack-arg=--keep \
462
+ "$url" >second.out &
463
+ second_pid=$!
464
+ } &&
465
+ test_when_finished "
466
+ kill $second_pid 2>/dev/null || :
467
+ wait $second_pid 2>/dev/null || :
468
+ " &&
469
+ wait "$second_pid" &&
470
+ wait "$first_pid" &&
471
+ wait "$server_pid" &&
472
+ printf "keep\t%s\npack\t%s\n" "$packhash" "$packhash" | sort >expect &&
473
+ sort first.out second.out >actual &&
474
+ test_cmp expect actual &&
475
+ test_path_is_missing "$tmpfile" &&
476
+ git -C packfileclient-overlap cat-file -e "$blob"
477
+'
478
+
479
test_expect_success 'fetch notices corrupt pack' '
480
cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
481
(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&