gvfs-helper: parallelize POST requests

[Derrick Stolee (derrickstolee)]

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 parallel implementation uses independent curl handles and index-pack --stdin children with a mutex-protected work queue. It preserves Git HTTP configuration, supports threadless builds through the sequential fallback, and avoids sending singleton loose-object responses to index-pack.

Dogfood testing exposed a deadlock caused by sibling index-pack processes inheriting one another's pipe descriptors. The series serializes child creation and marks parent pipe descriptors close-on-exec. It also removes the shared oid_to_hex() buffer race, protects SIGPIPE handling, isolates retry child lifecycles, closes error-path descriptors, and avoids deleting temporary packs belonging to concurrent processes using the shared ODB.

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

The focused t5798-gvfs-helper-post-threads.sh coverage exercises sequential and parallel requests, small batches, singleton remainders, duplicate downloads, and object integrity.

[tyrielv]

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.

[tyrielv]

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

[Derrick Stolee (derrickstolee)]

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.

[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]

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]

Note

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

[risk-rollout] 🟠 HIGH

Two fallback-semantics divergences from the sequential path:

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

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

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.

[tyrielv]

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:

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

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

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