Geo: Fix SSH proxy hang by closing pull request body after negotiation

Problem

Two separate bugs on the Geo SSH proxy pull path (internal/command/githttp/pull.go).

1. The proxied request body never closes, so slow transfers die. PullCommand.requestSSHUploadPack streamed the client's raw SSH stdin straight into the HTTP request body — a pattern copied from the push path, where it's correct. For pull, the request body carries small negotiation data, and the pack flows back on the response side. The body then sits idle on the primary for the whole transfer and eventually trips nginx's client_body_timeout. Raising that timeout only moves the failure point.

This causes The new Geo SSH proxy implementation hangs inde... (gitlab#584782 - closed).

2. Closing the body right after done breaks protocol v2 outright. Real git clients send a flush-pkt (0000) immediately after done. Under v0, done itself terminates negotiation, so stopping there is harmless. Under v2, done is only an argument line inside the fetch request section, and that section must be closed with a flush-pkt. Without it git upload-pack hits EOF and dies with fatal: the remote end hung up unexpectedly; Gitaly maps that stderr to a Canceled gRPC error; and the client sees every clone through the proxy fail with fatal: protocol error: bad line length character: fata.

Fix

readFromStdin now closes the request body once it has forwarded done, and injects a flush-pkt of its own before closing. Injecting rather than waiting to read the client's own flush is deliberate: under v0/v1 no flush ever follows done, so waiting would hang forever, while the extra flush is harmless there because upload-pack never reads past done.

This MR also extracts pipeUploadPack so requestUploadPack and requestSSHUploadPack share the pipe-and-copy logic, per Duo's review comment.

Why closing on done is sufficient

Only clients that send done hold the request body open. Measured with an instrumented SSH transport (git 2.53, 40 MB pack, throttled response):

Negotiation shape Client closes its write side Pack ends
v0 clone (sends done) 15.545s 15.560s
v2 fetch (server sends ready, no done) 0.082s 15.502s

Under v2, git half-closes as soon as it sees ready, so the request body reaches EOF on its own. Under v0 it stays open for the entire transfer — and v0 always sends done, which is what this fix closes on. Reproduced on a second, independent rig (close at 22.9ms, pack end at 27.0s).

A non-git v2 SSH client that never half-closes would still hold the body open; no such client is known.

Testing

Setup Protocol Result
3k Cloud Native Hybrid (Envoy Gateway ingress, Praefect-fronted Gitaly cluster) v2 Clone passed — failed before commit 8556cbd2
3k Cloud Native Hybrid (Envoy Gateway ingress, Praefect-fronted Gitaly cluster) v0 Passed — regression check for the injected flush
1k Omnibus (nginx, single Gitaly) v0 Passed, including git ls-remote, multi-round have negotiation, --depth 1, and git push
1k Omnibus (nginx, single Gitaly), AcceptEnv GIT_PROTOCOL on the secondary sshd v2 Clone passed
Unit test: v2 fetch fixture v2 Forwarded body matches what git sends, byte for byte
Standalone git upload-pack, with and without the trailing flush v2 Confirms the mechanism behind bug 2: dies with fatal: the remote end hung up unexpectedly without the flush, succeeds with it
1k Omnibus (nginx, single Gitaly), AcceptEnv GIT_PROTOCOL on the secondary sshd v2 With aincremental fetch (no done) and client_body_timeout at 5s, unpatched 190MB binary → passed

Slow-transfer verification for bug 1: a 700 MB clone through a throttled client with client_body_timeout forced to 5s failed at ~9s unpatched (unexpected EOF, workhorse logging context canceled at duration_ms: 5555) and completed in 53.4s patched.

Edited by Chloe Fons

Merge request reports

Loading
Loading