fix: accept thin packs in git receive-pack (HTTP + SSH)#47
Merged
Conversation
Routes go-git's receive-pack off the filesystem fast path onto NewParserWithStorage so REF_DELTAs against existing server objects can be resolved. See docs/superpowers/specs/2026-05-15-git-receive- thin-pack-fix-design.md.
Drives a go-git client push against an in-process server whose storer is wrapped by WrapForReceive, and verifies the push lands and links to pre-existing history. This is a transport smoke test, not a thin-pack bug reproduction: go-git's client never emits an external REF_DELTA base, so it cannot exercise the filesystem fast-path failure. Regression coverage for the bug lives in the TestWrapForReceive_HidesPackfileWriter unit test plus manual native-git verification.
Tracks pack size via an atomic counter. Used by both HTTP and SSH receive-pack handlers in a follow-up commit.
Pushes from native git clients producing thin packs now succeed (previously failed with 'reference delta not found' from go-git's filesystem-storer fast path). Also emit a structured slog.Info line with pack_bytes and duration_ms for slow-path visibility.
Symmetric change to the HTTP handler: wraps the server-side storer and emits the same six-field observability log line (owner, repo, pusher, commands, pack_bytes, duration_ms).
Captures why receive-pack routes the storer through gittransport.WrapForReceive, the trade-off (loose objects vs. correctness), and the observability surface.
internal/gittransport/storer_test.go imports go-git/go-billy/v5/memfs directly; go.mod now reflects that (previously // indirect via go-git). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to the thin-pack fix, addressing a code/security/silent-failure review: - M-1: reproduce the thin-pack bug in a hermetic test — a hand-crafted REF_DELTA pack that fails without WrapForReceive and passes with it — replacing manual-only verification. - M-2: cap pushed pack size via git.max_pack_bytes (default 2 GiB), enforced post-decompression on both HTTP and SSH so it bounds an oversized pack and a gzip bomb alike; over-limit pushes get HTTP 413 / a clear SSH error. Add a 60s SSH IdleTimeout. - M-3: add `cloudzilla gc` to prune loose objects unreachable from every ref and older than a grace period (internal/gitgc). - L-1: log refs_ok/refs_failed so partially-rejected pushes are visible. - L-2/L-3: document the deliberate SSH session Close suppression and tighten verbose comments. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- gitgc: refuse to prune a repo with zero ref roots — an empty root set made every loose object look unreachable and could empty the object DB - gitgc: reject symlinked .git/objects dirs so the GC cannot be steered into deleting outside the repository; skip non-regular object entries - gittransport: LimitedReadCloser returns (0, err) on the cap-crossing read — io.ReadFull cleared the error when the buffer was satisfied, letting an over-limit pack finish parsing as success - cmd/cloudzilla: validate `gc --repo` owner/name (rejects ../ escape); report skipped/failed counts; skip unreadable owner dirs - ssh: derive receive/upload-pack context from the session so a dropped client aborts ingestion - trim non-WHY comments across the changed files Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the two Medium residuals from review: - ssh: add git.ssh_max_session (default 2h, 0 disables) wired to ssh.Server.MaxTimeout — an absolute cap so a slow client trickling bytes cannot hold a connection past the idle timeout indefinitely. - gittransport: add AppliedCommands; both transports now run branch protection, webhooks, and post-receive only for refs go-git actually applied. A per-ref failure surfaces in the report status, not as a ReceivePack error, so unfiltered side effects fired for rejected refs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Native
git pushproduces thin packs by default — packs whose objects are delta-encoded against base objects already on the server. Cloudzilla's pure-Go receive-pack rejected them: go-git'sfilesystem.Storagetakes a fast path inpackfile.UpdateObjectStoragethat runs the pack parser without storage access, soREF_DELTAs with an external base can't be resolved. Pushes failed withreference delta not foundand HTTP 500 (the workaround was per-clonepack.window=0, unacceptable for production).This routes the receive-pack storer — on both the HTTP and SSH transports — through a new
gittransport.WrapForReceivewrapper that hides the storer'sPackfileWritermethod via interface-embedding. That forces go-git onto its slowerNewParserWithStoragepath, which can resolve external delta bases. The "no git binary required" invariant is preserved.Closes #
Changes
internal/gittransportpackage —WrapForReceive(interface-embedding storer wrapper) andByteCounter(atomic byte-countingio.ReadCloserfor observability).internal/handler/git_http.go) routes the storer throughWrapForReceiveand emits a structuredgit-http: receive-pack completelog line (pack_bytes,duration_ms).internal/ssh/server.go) — symmetric change;execGitServicenow threads owner/repo/pusher for the matchingssh: receive-pack completelog line.## Thin packssection indocs/git-transport.mddocumenting the rationale, trade-off (loose objects), and observability surface.go-billy/v5promoted to a direct dependency ingo.mod(the new package's unit test importsmemfsdirectly).WrapForReceivehidesPackfileWriter), a transport smoke test, andByteCountertests (incl. concurrency under-race).Notes for reviewers
REF_DELTAbase, so it can't trigger the server-side failure. The load-bearing regression guard is theTestWrapForReceive_HidesPackfileWriterunit invariant; end-to-end behavior needs manual verification with a native git client (still pending).Type of change
bug/...)feat/...)tech/...)docs/...)Checklist
make lintpasses with no issues —make lintreports 49 issues, all pre-existing onmain;golangci-lint --new-from-rev=mainconfirms 0 introduced by this PR (the repo has no.golangci.ymland CI doesn't run golangci-lint).go test ./...passes.templfiles changed — n/afix:)