git@cosmocr.at/gitweb.git

git@cosmocr.at/gitweb.git · summary refs

docs: FR for undiagnosable 400 on pushes over ~1 MiB

commit ab6f418ff5657b7d436fae5c25f47e176833e1d9

Patrick Gallagher <rpg137@gmail.com> · 2026-09-20 14:21 -0400

Pushing a repo whose pack exceeds git's default http.postBuffer fails with
an opaque "400 bad request"; the client-side workaround is to raise
postBuffer per-repo, which nothing in the response hints at.

The filed defect is deliberately the observability one rather than a root
cause: internal/server/githttp.go collapses every DecodeReceivePack failure
into one string and discards the error, while every sibling branch in the
same handler logs. That is what made the incident take an hour and still
not reach an answer.

Records what was ruled out with evidence so it is not re-investigated.
Chunked transfer-encoding decodes correctly at both minimal and 6 MiB
sizes, so the obvious theory ("gitweb cannot parse chunked") is false and
acting on it would have aimed the fix the wrong way; the regression test
proving that is included in the FR and passes as written.

A controlled repro on demo.git (688 KiB, unrelated to the original
incident) used two branches identical except for ancestry, each carrying a
3 MiB incompressible payload:

  size-only   (shares an ancestor)  default postBuffer     -> HTTP 400
  no-ancestor (orphan)              default postBuffer     -> HTTP 400
  size-only                         postBuffer=500 MiB     -> succeeded

Pack size alone is sufficient. The size-only branch has no external delta
bases, so thin-pack resolution is not involved and it fails anyway; that
hypothesis is struck. A force-push rewinding main also succeeded at the
default postBuffer, so ref-update handling is not implicated either — only
the large-pack path.

Expect: 100-continue is the leading remaining suspect: it is specific to
the large/streamed request path and independent of pack contents, which is
exactly the shape of the evidence. Deployed-binary drift remains untested.
The reproduction recipe is the demo-repo one actually used, and notes the
push.default=upstream trap in that clone, which sent a repro branch to main
during this investigation.

added docs/FR-large-push-400.md
+	# FR: pushes over ~1 MiB fail with an undiagnosable `400 bad request`
+
+	Status: open — root cause narrowed, not yet identified
+	Filed: 2026-09-19
+	Observed on: `git.cosmocr.at` (deployed), pushing `rpg137@gmail.com/einkdashboards.git`
+	Reproduced on: `git@cosmocr.at/demo.git` with a 3 MiB payload (see below)
+
+	## Symptom
+
+	Pushing a repository whose pack exceeds git's default `http.postBuffer` (1 MiB)
+	fails:
+
+	```
+	$ git push --force origin master multi-dashboard
+	error: RPC failed; HTTP 400 curl 22 The requested URL returned error: 400
+	fatal: the remote end hung up unexpectedly
+	Everything up-to-date
+	```
+
+	`git fetch` against the same repo, over the same connection, with the same
+	credentials, succeeds immediately beforehand — so this is not auth, TLS,
+	routing, or repo existence.
+
+	The workaround is client-side:
+
+	```bash
+	git config http.postBuffer 524288000 # 500 MiB
+	```
+
+	After which the identical push succeeds. Every user has to discover and apply
+	this per-repo, and nothing in the server's response hints at it.
+
+	## What is actually wrong
+
+	The primary defect is that the failure is unattributable. In
+	`internal/server/githttp.go:140-143`:
+
+	```go
+	case gitReceivePack:
+	req, err := githttp.DecodeReceivePack(r)
+	if err != nil {
+	http.Error(w, "400 bad request", http.StatusBadRequest)
+	return
+	}
+	```
+
+	Every possible decode failure — malformed pkt-line, truncated body, a pack
+	go-git cannot resolve, a body over `maxBody`, a read error mid-stream —
+	collapses into the same opaque string, and **`err` is discarded without being
+	logged**. Every other branch in this handler logs (`log.Printf("gitweb:
+	upload-pack %s: %v", ...)`); this one does not.
+
+	Consequence: the server operator cannot tell a client-side protocol error from
+	a server-side bug, and the client is told nothing actionable. Diagnosing the
+	incident above took roughly an hour and still did not reach a root cause.
+
+	## What has been ruled out
+
+	Investigated and eliminated, so nobody repeats the work:
+
+	\| Hypothesis \| Verdict \|
+	\|---\|---\|
+	\| nginx rejecting the body \| No. nginx does SNI passthrough (`docs/ARCHITECTURE.md:53`); it routes TCP and never parses HTTP, so `client_max_body_size` cannot apply. \|
+	\| `maxBody` cap in `requestBody` \| No. 256 MiB (`internal/githttp/githttp.go:34`); the failing pack was ~5.7 MiB. \|
+	\| `MaxRepoBytes` \| No. 256 MiB default (`main.go:57`), and it is enforced after decode. \|
+	\| Server read timeout cutting off a slow upload \| No. Only `ReadHeaderTimeout` is set (`main.go:112`); there is no `ReadTimeout`. \|
+	\| Chunked transfer-encoding decoding \| No — tested. `DecodeReceivePack` handles a chunked body correctly, at both minimal size and 6 MiB. See the test below. \|
+
+	That last row matters: the obvious theory ("gitweb can't parse chunked
+	requests") is false, and an FR that asserted it would have sent the fix in
+	the wrong direction. The chunked path decodes; something else about a real
+	large push does not.
+
+	## Controlled reproduction (2026-09-19)
+
+	Reproduced on `git@cosmocr.at/demo.git` — a 688 KiB repo unrelated to the
+	original incident — using two branches identical except for ancestry, each
+	carrying a 3 MiB incompressible payload (random data; zeros would deflate away
+	and never exceed `postBuffer` on the wire).
+
+	\| Branch \| Common ancestor with `main` \| Pack \| `http.postBuffer` \| Result \|
+	\|---\|---\|---\|---\|---\|
+	\| `size-only` \| yes \| 3.00 MiB \| default (1 MiB) \| HTTP 400 \|
+	\| `no-ancestor` (orphan) \| no \| 3.00 MiB \| default (1 MiB) \| HTTP 400 \|
+	\| `size-only` \| yes \| 3.00 MiB \| 500 MiB \| succeeded \|
+
+	Client error is byte-identical to the original incident:
+
+	```
+	error: RPC failed; HTTP 400 curl 22 The requested URL returned error: 400
+	fatal: the remote end hung up unexpectedly
+	```
+
+	Conclusion: pack size alone is sufficient. The `size-only` branch shares an
+	ancestor with `main`, so no external delta bases and no thin-pack resolution
+	are involved, and it fails anyway. Ancestry makes no difference — both shapes
+	fail identically, and the same objects push cleanly once the client stops using
+	chunked encoding.
+
+	This demotes the thin-pack hypothesis below. The trigger is a pack exceeding
+	`http.postBuffer`, and the fix belongs in request handling rather than the pack
+	resolution path.
+
+	Also observed in the same session: a force-push rewinding `main` succeeded
+	at the default `postBuffer` (it sends a ref update and no new objects). Ref
+	update handling is therefore not implicated; only the large-pack path fails.
+
+	## Remaining hypotheses
+
+	In rough order of likelihood, after the reproduction above:
+
+	1. ~~Thin packs.~~ Ruled out by the controlled reproduction — a push
+	sharing an ancestor, with no external delta bases, fails identically.
+	2. `Expect: 100-continue` handling. Now the leading suspect. curl adds the
+	header for large streamed bodies; Go sends `100 Continue` only when the
+	handler first reads the body, so any path that writes a response before
+	reading would strand the client. This fits the evidence: it is specific to
+	the large/streamed request path and independent of pack contents.
+	3. Deployed binary drift. `HEAD` here is `5444040` (2026-07-12) on branch
+	`sso-browser-login`. Whether the running service matches this source is
+	unconfirmed — the bug may already be fixed in tree. Worth eliminating
+	cheaply before any code change, since the unit tests in this repo show the
+	decode path handling chunked bodies correctly.
+
+	## Proposed change
+
+	### 1. Log the error (do this first, independently of any fix)
+
+	```go
+	req, err := githttp.DecodeReceivePack(r)
+	if err != nil {
+	log.Printf("gitweb: receive-pack decode %s: %v", repoPath, err)
+	http.Error(w, "400 bad request", http.StatusBadRequest)
+	return
+	}
+	```
+
+	One line. It converts this class of incident from guesswork into a log grep,
+	and it is what tells us which of the hypotheses above is real.
+
+	### 2. Return a useful body to the client
+
+	git surfaces the response body on `remote:` lines. Distinguish the cases that
+	have distinct remedies — at minimum, separate "body too large" (413, with the
+	limit named) from "malformed request" (400). A client seeing
+	`remote: pack exceeds 256 MiB limit` needs no server log to self-serve.
+
+	### 3. Fix the underlying failure
+
+	Only once (1) identifies it. If it is thin packs, the fix belongs in the pack
+	resolution path, not in request handling — and no amount of `postBuffer` tuning
+	is the right answer for users.
+
+	## How to test
+
+	### Regression test for the chunked path (passes today — keep it that way)
+
+	Add to `internal/githttp/githttp_test.go`. This is the test that disproved the
+	original theory; it is worth keeping so the chunked path does not silently
+	regress:
+
+	```go
+	// TestDecodeReceivePackChunked drives a chunked body through a real
+	// http.Server, which is what git sends once a request exceeds
+	// http.postBuffer (default 1 MiB).
+	func TestDecodeReceivePackChunked(t *testing.T) {
+	const (
+	oldOID = "0000000000000000000000000000000000000000"
+	newOID = "1111111111111111111111111111111111111111"
+	)
+	cmd := fmt.Sprintf("%s %s refs/heads/master\x00report-status\n", oldOID, newOID)
+	var body bytes.Buffer
+	fmt.Fprintf(&body, "%04x%s", len(cmd)+4, cmd)
+	body.WriteString("0000")
+	body.WriteString("PACK")
+	body.Write([]byte{0, 0, 0, 2, 0, 0, 0, 0})
+	body.Write(bytes.Repeat([]byte{0xAB}, 6<<20)) // past postBuffer
+	body.Write(make([]byte, 20))
+
+	errCh := make(chan error, 1)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	if r.ContentLength != -1 {
+	t.Errorf("ContentLength = %d, want -1 (chunked)", r.ContentLength)
+	}
+	req, err := DecodeReceivePack(r)
+	if err == nil && req.Packfile != nil {
+	_, err = io.Copy(io.Discard, req.Packfile)
+	}
+	errCh <- err
+	}))
+	defer srv.Close()
+
+	hreq, _ := http.NewRequest(http.MethodPost, srv.URL+"/git-receive-pack",
+	io.NopCloser(bytes.NewReader(body.Bytes())))
+	hreq.ContentLength = -1 // force chunked
+	resp, err := http.DefaultClient.Do(hreq)
+	if err != nil {
+	t.Fatalf("request failed: %v", err)
+	}
+	defer resp.Body.Close()
+	if err := <-errCh; err != nil {
+	t.Fatalf("chunked body rejected: %v", err)
+	}
+	}
+	```
+
+	Run: `go test ./internal/githttp/ -run Chunked -v`
+
+	### End-to-end reproduction (the one that actually fails)
+
+	A unit test cannot produce a real git pack. This recipe reproduces it on the
+	demo repo in under a minute; it is the one used for the results above.
+
+	```bash
+	git clone https://git.cosmocr.at/git@cosmocr.at/demo.git /tmp/repro
+	cd /tmp/repro
+
+	# Push to a scratch ref with an EXPLICIT refspec. This clone ships
+	# push.default=upstream, so a bare `git push origin <branch>` targets the
+	# branch's upstream (main) rather than creating a same-named branch —
+	# that mistake polluted main once already.
+	git checkout -b fr-repro
+
+	# Incompressible payload: zeros would deflate away and never exceed
+	# postBuffer on the wire.
+	mkdir -p fr-repro && head -c 3145728 /dev/urandom > fr-repro/payload-3mb.bin
+	git add fr-repro && git commit -qm "FR-large-push-400 repro payload"
+
+	git config --unset http.postBuffer 2>/dev/null # ensure the 1 MiB default
+
+	# Expected: HTTP 400.
+	GIT_CURL_VERBOSE=1 git push origin fr-repro:refs/heads/fr-repro 2>&1 \| tee /tmp/push.log
+
+	# With the logging change in place, the server log now names the cause:
+	sudo docker logs --tail 50 patrick_gitweb_1 \| grep 'receive-pack decode'
+
+	# Control: identical objects, non-chunked request. Expected: success.
+	git -c http.postBuffer=524288000 push origin fr-repro:refs/heads/fr-repro
+
+	# Clean up the scratch ref.
+	git push origin :refs/heads/fr-repro
+	```
+
+	To also confirm ancestry is irrelevant, repeat with `git checkout --orphan`
+	instead of `git checkout -b`; both shapes fail identically.
+
+	`GIT_CURL_VERBOSE=1` shows whether the request went out chunked and whether
+	`Expect: 100-continue` was used — which settles hypothesis 2 directly.
+
+	### Acceptance criteria
+
+	- A failing push writes a specific, actionable reason to the server log.
+	- The client receives a response body distinguishing "too large" from
+	"malformed".
+	- A >1 MiB push succeeds at the default `http.postBuffer`, with no client-side
+	configuration, for both normal and orphan history.
+	- `go test ./...` green, chunked regression test included.
+
+	## Impact
+
+	Low frequency, high friction. It only bites repos over ~1 MiB, but when it does
+	the failure is opaque, the fix is non-obvious, and the workaround has to be
+	repeated per-repo per-clone. Of the repos currently hosted, `einkdashboards`
+	(5.7 MiB) trips it; `mtadash` (328 KiB) does not, which is why this went
+	unnoticed until now.