gvfs-helper: parallelize POST requests by derrickstolee · Pull Request #980 · microsoft/git

Derrick Stolee (derrickstolee) · 2026-08-24T13:40:19Z

Improve full-clone performance by allowing gvfs-helper post to download
object batches concurrently. The new gvfs.postThreads configuration
defaults to 1, preserving the existing sequential behavior unless explicitly
enabled.

The series is organized into six reviewable commits:

Add the configuration surface.
Factor reusable curl-handle preparation.
Introduce the parallel POST success path.
Complete retry, authentication, fallback, throttling, and pack
installation behavior.
Test sequential and parallel success paths.
Test failure handling and request-header parity.

Each worker owns an independently prepared curl handle and streams responses
into a dedicated index-pack --stdin child. Work is distributed through a
mutex-protected queue, with the worker count capped by the number of object
batches. Requests with configured cookies retain the established sequential
path because libcurl cookie state cannot safely be shared by concurrently
performing handles.

The parallel path preserves the sequential HTTP policy for authentication
refresh, transient errors, Retry-After, cache and backup-cache fallback,
--no-fallback, configured headers, X-Session-Id, and X-VSS-E2EID.
Sequential and parallel requests share response-header parsing, while
soft-throttle state remains local to each worker.

Child pipes are created and marked close-on-exec before spawning, and child
setup and completion are serialized around process-global run-command state.
Each index-pack attempt uses unique pack and index paths, validates its
reported pack hash, retries corrupt or truncated responses, and tolerates
another process winning installation of the same final pack.

The focused t5798-gvfs-helper-post-threads.sh suite contains 22 tests
covering configuration boundaries, sequential and parallel requests,
multi-worker participation, singleton remainders, duplicate downloads,
deadlock prevention, authentication, throttling, cache fallback,
--no-fallback, corrupt packs, cookies, and configured headers. Parallel
tests use pthread prerequisites and Trace2 assertions to prove the intended
execution path.

Neil Kainga diagnosed and tested the pipe-inheritance fix on a 1JS full clone
with gvfs.postThreads=8 and is credited throughout the series.

tyrielv · 2026-08-24T15:45:52Z

 	installation of multiple prefetch packs. Values less than `1` are
 	treated as `1`.
+
+gvfs.postThreads::


Does this feature ever tend to bottleneck on CPU (ie when indexing pack files) or only on network/remote?

I'm wondering if there'd be any benefit or downside to supporting "values less than 1 are treated as NUMBER_OF_PROCESSORS" like checkout.workers does.

Also, GVFS uses Environment.ProcessorCount as the default parallelism value for its analogous workflow (gvfs prefetch --files or --folders)

We could consider the "values less than one" option. I worry that the network will saturate at a lower parallelism than the CPU doing pack-indexing.

I do agree that number of CPU cores has little to do with the optimal value; We cannot determine the optimal value based on anything Git can inspect, therefore the default of 1 (and no logic to auto-select an appropriate number for 0) sounds good to me.

tyrielv

Automated multi-perspective review (Review Swarm): six independent reviewer lenses run on diverse models, findings deduplicated and human-reviewed before posting. Four comments follow. The concurrency scaffolding itself held up well under review — fd lifecycles, the CLOEXEC/spawn_mutex deadlock fix, per-thread curl handle cleanup, and the block partitioner all look sound, and no TLS or credential weakening was found. The comments concentrate on behavioral parity between the new parallel path and the existing sequential one.

tyrielv · 2026-09-01T16:47:11Z

+				continue;
+			}
+
+			if (res != CURLE_OK)


Note
🤖 Machine-drafted review comment (Review Swarm) — reviewed & approved by tyrielv before posting

[correctness + risk-rollout + security — convergent across 3 independent model families] 🟠 HIGH

The threaded POST path calls curl_easy_perform directly and collapses every non-200/non-404 response into a fatal GH__ERROR_CODE__INDEX_PACK_FAILED, then breaks. The sequential path (gh__response_status / GH__RETRY_MODE__*, ~898-936 and ~3496-3573) handles 401 by reloading main credentials and re-authenticating, and 429/503 as transient retries with backoff and Retry-After.

The parallel worker does none of this. An expired auth token or server throttling — both routine against real GVFS/Azure endpoints, which is exactly the dogfooding target — aborts the whole post where today's code recovers. Two aggravating details:

HTTP/curl failures are surfaced as index-pack failed, which will mislead anyone triaging from telemetry.

Because workers never call credential_reject/credential_approve, each worker keeps sending the same rejected secret concurrently and the bad credential is never invalidated in the helper.

This changes the failure semantics of gvfs-helper post, not just its speed, for anyone who sets gvfs.postThreads > 1.

tyrielv · 2026-09-01T16:47:12Z

+		 * Start a fresh index-pack before falling back to the main
+		 * server so the two response bodies cannot be concatenated.
+		 */
+		if (can_fallback &&


Note
🤖 Machine-drafted review comment (Review Swarm) — reviewed & approved by tyrielv before posting

[risk-rollout] 🟠 HIGH

Two fallback-semantics divergences from the sequential path:

Cache-server 404 no longer falls back to origin. It is excluded from fallback and instead retried (up to six times, with delays that synchronize across workers) against the same cache server. The sequential path falls back to origin on a cache 404. Cold-cache objects therefore become partial failures precisely under the workload parallel mode was built for, and the synchronized retries multiply load on an already-missing cache.

gvfs.fallback=false / --no-fallback appears to be ignored. At the if (gh__global.cache_server_url) block in do__http_post__fetch_oidset (~4416), an origin fallback URL is built whenever a cache URL exists, without consulting gh__cmd_opts.try_fallback. That would defeat an explicit origin-load containment control operators rely on during an incident.

tyrielv · 2026-09-01T16:47:12Z

+ * their final locations.  Tolerates races where another thread or
+ * process installed the same packfile concurrently.
+ */
+static int my_finalize_packfile_simple(const char *temp_pack,


Note
🤖 Machine-drafted review comment (Review Swarm) — reviewed & approved by tyrielv before posting

[design] 🟡 MEDIUM

The doc comment says this tolerates races where another thread or process installed the same packfile concurrently, but I don't think it does.

my_finalize_packfile has an explicit assume_ok path: when finalize_object_file_flags fails, it checks file_exists(final_path_pack) && file_exists(final_path_idx) and treats that as success. my_finalize_packfile_simple returns -1 instead, and the caller (~4309) turns that into GH__ERROR_CODE__INDEX_PACK_FAILED and breaks out of the worker loop — failing the whole post.

That race seems most likely on exactly this path: two workers producing the same pack hash, or two gvfs-helper processes sharing an ODB, which the PR description calls out. The narrower signature looks deliberate and right — dropping status in particular makes sense since it isn't thread-safe. It's just this one property that the comment promises and the body doesn't implement.

I'm not sure that I agree.

When a .pack file exists with a corresponding .idx file in place (which is always renamed atomically from a temporary file, after its contents have been built up), Git's contract is that it is now usable.

So I do think that the file_exists() && file_exists() check for .pack/.idx is sufficient to prove that a concurrent download succeeded.

Obviously, it would be even better to prevent concurrent downloads from trying to produce identical .pack files (which, thanks to encoding the contents' SHA in the filename, means that those concurrent downloads wanted to download the very same objects in the very same order), but I don't think that we can realistically enforce that, not when we consider that parallel Git invocations can, and will, ask for the very same object sets.

tyrielv · 2026-09-01T16:47:12Z

+	test_expect_success "post blobs ($mode, threads=$threads)" '
+		test_when_finished "per_test_cleanup" &&
+		start_gvfs_protocol_server &&
+		git -C "$REPO_T1" config gvfs.postThreads '$threads' &&


Note
🤖 Machine-drafted review comment (Review Swarm) — reviewed & approved by tyrielv before posting

[tests] 🟠 HIGH

Three gaps, the first of which makes the new coverage largely nominal:

The parallel cases never verify the parallel path ran. They set gvfs.postThreads=4 and export GIT_TRACE2_EVENT, but never read the trace back. The sibling t5797-gvfs-helper-prefetch-threads.sh asserts test_trace2_data gvfs-helper prefetch/install_mode $expected_mode after every parallel test. Without that, if HAVE_THREADS is 0 or the entry gate rejects the batch (nr_oids <= 1, block_size <= 1), the sequential fallback runs and all four tests still pass. Relatedly there is no HAVE_THREADS prerequisite, so a threadless build reports parallel tests pass with no parallel code executed.

No regression test for the pipe-inheritance deadlock — the bug that motivated the series. start_command_cloexec under spawn_mutex is subtle enough that a future refactor could revert it silently. Such a test needs threads >= 2, enough batches for >= 2 concurrent index-pack spawns, and a hard timeout — otherwise a regression hangs CI instead of failing.

No error-path coverage. t5797 has do_prefetch_corrupt_pack via start_gvfs_protocol_server_with_mayhem; t5798 has no analogue. Nothing exercises a worker's index-pack failing, a non-200, or a mid-stream close — so post_worker_thread_fn's error aggregation (td->ec, td->error_message) is entirely untested.

Johannes Schindelin (dscho) · 2026-09-03T12:18:07Z

+	enum gh__error_code ec;
+	struct strbuf error_message;
+	struct string_list result_list;
+	int had_404;


This is probably fine for now. In the long run, we may want to consider storing an error condition as an enum from a range of eligible conditions.

Johannes Schindelin (dscho) · 2026-09-03T12:29:04Z

+	if (!ret) {
+		if (cp->in > 0)
+			fcntl(cp->in, F_SETFD,
+			      fcntl(cp->in, F_GETFD) | FD_CLOEXEC);


We probably want to handle fcntl() returning -1 (see docs).

Johannes Schindelin (dscho) · 2026-09-03T12:31:15Z

+		struct strbuf final_pack = STRBUF_INIT;
+		struct strbuf final_idx = STRBUF_INIT;
+		struct strbuf final_name = STRBUF_INIT;
+		unsigned long block_start;


It might be a good idea to use size_t here instead of unsigned long.

Johannes Schindelin (dscho) · 2026-09-03T12:32:59Z

+			break;
+		}
+
+		ip_stdin_fd = ip.in;


Would it be better to create the pipe with CLOEXEC outside of start_command(), then just hand it to that function by assigning ip.in = ip_stdin_fd instead? That would render the start_command_cloexec() function obsolete.

Johannes Schindelin (dscho) · 2026-09-03T12:37:23Z

+			if (hash_hex)
+				hash_hex++;
+			else
+				hash_hex = ip_stdout.buf;


This would result in pack-.pack to be written in case no hash was detected; I believe we should error out in that instance instead.

Johannes Schindelin (dscho) · 2026-09-03T12:39:31Z

 	if (!nr_oid_total)
 		return;

+	if (HAVE_THREADS && gh__global.post_threads > 1 &&


This is a pretty theoretical nit, as we only provide Microsoft Git on platforms that do have threads: In case threads are disabled, we should maybe warn for postThreads > 1?

Johannes Schindelin (dscho) · 2026-09-03T12:42:49Z


+	if (HAVE_THREADS && gh__global.post_threads > 1 &&
+	    gh__cmd_opts.block_size > 1 && nr_oid_total > 1 &&
+	    (gh__cmd_opts.block_size > 2 || !(nr_oid_total & 1))) {


Hmm. I don't quite understand the condition block_size > 2 || nr_oid_total is even... Wouldn't that also catch nr_oid_total == 0, in theory? Also, what's the ELI5 version of this condition?

Johannes Schindelin (dscho) · 2026-09-03T12:47:05Z

+		ALLOC_ARRAY(oid_array, nr_oid_total);
+		oidset_iter_init(oids, &iter);
+		for (k = 0; (oid = oidset_iter_next(&iter)); k++)
+			oidcpy(&oid_array[k], oid);


A slightly more memory-efficient method would be to store only pointers in oid_array, seeing as oids owns the OIDs and its lifetime must completely enclose the lifetime of the worker threads.

Johannes Schindelin (dscho) · 2026-09-03T12:50:18Z

-		/*
-		 * Because the oidset iterator has random
-		 * order, it does no good to say the k-th or
-		 * n-th chunk was incomplete; the client
-		 * cannot use that index for anything.
-		 *
-		 * We get a 404 when at least one object in
-		 * the chunk was not found.
-		 *
-		 * For now, ignore the 404 and go on to the
-		 * next chunk and then fixup the 'ec' later.
-		 */


Is this comment no longer applicable? Otherwise please tell your AI to refrain from such undesirable drive-by changes ;-)

Johannes Schindelin (dscho) · 2026-09-03T12:51:34Z

  't5794-gvfs-helper-packfiles.sh',
  't5795-gvfs-helper-verb-cache.sh',
  't5797-gvfs-helper-prefetch-threads.sh',
+  't5798-gvfs-helper-post-threads.sh',


This is repeated in the commit message. In general, I prefer to reign in LLMs when they repeat facts that are obvious from the diff anyway.

Johannes Schindelin (dscho)

In addition to the comments I left, I do want to explicitly agree with 69ec523#r3906355140 and 69ec523#r3906355124 as concerns that most likely need code changes.

Prepare the configuration surface for parallel POST workers before the worker implementation is introduced. Add gvfs.postThreads with a default of one so this commit does not change request execution on its own. Document the intended concurrent behavior and clamp values below one. Later commits in the series consume the value while introducing the parallel success path and then its complete failure handling. Helped-by: GPT-5.6 Sol Co-authored-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Derrick Stolee <stolee@gmail.com>

The parallel POST implementation needs standalone curl handles with the same runtime settings as handles allocated through get_active_slot(). Creating raw handles would otherwise omit cookies, configured host resolutions, redirect policy, IP selection, and current authentication defaults. Extract the per-request handle preparation into a shared helper and use it from get_active_slot(). Expose a function that duplicates the initialized default handle and applies the same preparation for callers that manage a handle outside the active-slot machinery. Also expose whether cookies are configured. Libcurl cannot safely share cookie state across concurrently performing handles, so callers can retain an established sequential path in that case. Helped-by: GPT-5.6 Sol Co-authored-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Derrick Stolee <stolee@gmail.com>

Fetching a large set of missing objects through gvfs-helper performs each HTTP POST and index-pack operation sequentially. This leaves the client waiting on individual network transfers even when the server and local machine can support concurrent work. Introduce the parallel success-path mechanism. Use a mutex-protected queue to distribute full object batches across worker threads. Each worker owns a curl handle and streams each response into a fresh index-pack process. Serialize child startup while marking pipe descriptors close-on-exec so concurrent index-pack children cannot keep sibling pipes open. Keep OID formatting and result collection thread-local, and partition work into batches containing at least two objects because a single non-commit object can be returned loose instead of as a pack. This commit deliberately establishes the core worker and transfer mechanics first. The next commit completes authentication, throttling, fallback, retry, and concurrent pack installation behavior before tests exercise the new path. Helped-by: GPT-5.6 Sol Co-authored-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Derrick Stolee <stolee@gmail.com>

The initial parallel POST path handles successful requests but does not yet match the sequential path when authentication, throttling, corrupt responses, or concurrent pack installation interfere with a request. Those differences can turn recoverable network failures into hard errors or allow sibling processes to manipulate the same pack paths. Classify HTTP and curl failures with the existing retry rules, refresh credentials outside worker threads, and preserve the cache, backup cache, and origin fallback order. Share response-header parsing with the sequential path so workers retain rate-limit telemetry while keeping soft-throttle state local to each worker. Coordinate Retry-After delays across workers without overflowing sleep intervals, and wait before starting index-pack. Serialize child setup and completion because finish_command() invalidates process-global path state. Limit the worker count to the number of queued object batches. Give each index-pack attempt unique pack and index paths, validate its reported pack hash, and retry corrupt or truncated responses. A complete final pack and index pair remains sufficient when another process wins the installation race. Group the per-attempt buffers behind one cleanup helper so success, retry, fallback, and failure paths release the same state. Helped-by: GPT-5.6 Sol Co-authored-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Derrick Stolee <stolee@gmail.com>

Exercise gvfs-helper POST requests with both one and four configured workers so the sequential and parallel paths must fetch identical object sets. Cover multiple batches, a final single-OID remainder, and duplicate requests while checking both installed objects and packfile counts. Require pthread support for parallel cases and use Trace2 assertions to prove that each test reaches its intended execution mode. Helped-by: GPT-5.6 Sol Co-authored-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Derrick Stolee <stolee@gmail.com>

Parallel requests need to preserve the sequential path's behavior for configuration boundaries, authentication, throttling, cache fallback, corrupt responses, and request headers. Extend the protocol test server with targeted failure modes. Verify that parallel POST refreshes authentication, honors Retry-After, falls back from cache 404 responses only when permitted, retries a one-time corrupt pack, and reports permanent corruption as an index-pack failure. Also cover absent and invalid thread configuration, cookie-enabled sequential fallback, configured headers, multiple participating workers, and a timeout-protected child-pipe stress case. Helped-by: GPT-5.6 Sol Co-authored-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Neil Kainga <t-neilkainga@microsoft.com> Signed-off-by: Derrick Stolee <stolee@gmail.com>

Derrick Stolee (derrickstolee) · 2026-09-04T19:18:33Z

I'll do a manual recheck of Johannes' comments next week.

tyrielv reviewed Aug 24, 2026

View reviewed changes

Derrick Stolee (derrickstolee) force-pushed the parallel-post-threads branch from f05b1d5 to 6b263d8 Compare August 24, 2026 17:16

tyrielv reviewed Sep 1, 2026

View reviewed changes

Johannes Schindelin (dscho) reviewed Sep 3, 2026

View reviewed changes

Derrick Stolee (derrickstolee) and others added 6 commits September 4, 2026 11:33

Derrick Stolee (derrickstolee) force-pushed the parallel-post-threads branch from 6b263d8 to 8a77b60 Compare September 4, 2026 18:21

Uh oh!

Conversation

Derrick Stolee (derrickstolee) commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyrielv Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyrielv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Johannes Schindelin (dscho) left a comment

Choose a reason for hiding this comment

Uh oh!

Derrick Stolee (derrickstolee) commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Derrick Stolee (derrickstolee) commented Aug 24, 2026 •

edited

Loading

tyrielv Aug 24, 2026 •

edited

Loading