Sure. Here's the same argument in plainer terms.

The setup

git-sync has two ways to push to a target:

• relay — incremental, cheap, but only works in certain conditions
• materialized — full rebuild, more expensive, used as a fallback

You ran two syncs in sequence:

1. one with --tags
2. one with --all-refs

The second one took the materialized path and hit a bug in materialized's HTTP code. The question was: did step 1 cause step 2 to fail?

The claim

No — step 2 would have taken the materialized path regardless, because of what --all-refs itself does, not because step 1 ran first.

Why --all-refs is the real cause

Relay can only handle "normal" refs — refs/heads/* and refs/tags/*. If a sync includes anything else (e.g. refs/notes/*, refs/pull/*, custom namespaces), the planner says "relay can't do this" and falls back to materialized.

--tags stays inside the safe set. --all-refs opens the door to everything, so if the source repo has any non-standard refs, relay is immediately disqualified.

Why step 1 wasn't innocent either, but only marginally

Before step 1, the target was empty. After step 1, the target had content. Planning logic can behave differently against an empty vs. non-empty target — so step 1 did change the situation, just not in the way that mattered here. The decisive factor was the ref shape in step 2, not the target state left by step 1.

The takeaway

• "I ran --tags first, that's why it broke" → wrong framing
• "--all-refs exposed refs that disqualify relay, so it fell back to a broken materialized path" → right framing

Want me to read internal/strategy/ and confirm which ref namespaces actually trigger the materialized fallback? That would turn the explanation from "this is how it generally works" into "this is exactly what's happening on your repo."

OK, now I can answer with the actual reason. The code is at internal/planner/relay.go:110-111 — relay refuses any ref that isn't a branch or tag, and --all-refs is what brings in those "other" refs.

The design tradeoff

Relay is built around fast-forward safety. It only handles two ref shapes:

• branch updates that are provably fast-forwards (target hash is an ancestor of source hash)
• branch creates and tag creates (target has no prior ref, so nothing to validate)

That covers the common case cheaply: relay can verify a small set of hashes and forward the push without rebuilding anything.

The problem with "other" refs (refs/notes/*, refs/pull/*, replace refs, custom namespaces) is they don't behave like branches:

• notes get rewritten non-FF all the time — that's the normal mode of operation
• pull refs are server-managed and can change shape arbitrarily
• custom namespaces have no consistent semantics at all

So if relay tried to handle them, it would either:

• reject perfectly valid updates because they aren't fast-forwards, or
• skip the FF check and risk silently dropping data

Rather than build per-namespace logic for every kind of ref, the planner takes the conservative route: if the scope includes anything outside heads/tags, hand the whole job to materialized. Materialized doesn't need FF assumptions because it rebuilds the target from scratch — by construction the result is whatever the source says, no merge semantics required.

Why this is reasonable

• relay is an optimization, not a correctness path
• the common case (mirror branches and tags) stays fast
• the rare case (--all-refs with weird namespaces) pays the cost of a full materialize
• you don't have to teach relay about every ref convention in the wild

Why it feels wrong here

The fallback is correct in principle but currently lands on the broken materialized-over-HTTP path. So the design isn't really at fault — the symptom is "materialized push is broken," and --all-refs just happens to be the trigger that exposes it. The fix belongs in materialized's HTTP target, not in making relay handle "other" refs.

Actually your intuition is exactly right — and the code already encodes it, just not in the way you'd expect. Look at internal/planner/relay.go:

--force in sync mode disables relay entirely (line 75):

So in sync mode, force does the opposite of what you'd hope — it kicks you off the relay path. Surprising at first, but it makes sense once you see the second piece.

CanReplicateRelay (line 148) handles RefKindOther happily — and the comment at lines 166–168 is essentially your argument, in the codebase:

So the codebase already splits the world the way you're describing, but at the mode level rather than at a flag level:

Why the split exists

The distinction is that --force in sync is a per-invocation escape hatch on an otherwise FF-safe contract, while replicate is a separate mode whose entire purpose is "make target look like source." The relay code was written to optimize the contract, not the flag. So replicate got the "other refs are fine, just relay them" carve-out; sync's --force did not.

You could in principle extend CanIncrementalRelay so force=true follows the same logic as replicate for RefKindOther. The reason that hasn't been done is probably:

• --force in sync is rare and historically meant "let me FF over a divergent branch," not "push arbitrary namespaces"
• the materialized fallback was assumed to work, so widening relay wasn't worth the risk
• replicate already exists for the "overwrite everything" use case — if you want that semantic, the recommendation would be to use replicate mode rather than --force --all-refs in sync

So: yes, force could in theory let relay handle other-kind refs, the architectural reasoning for why force should be safe is already written into replicate. It just hasn't been wired into sync's --force path. If your real goal is "mirror everything including notes/pulls without going through materialized," replicate mode is the existing answer.

Great instinct to look at git itself. The short answer is: you're right — a ref has zero FF semantics, it's just a pointer. FF is a policy that the client and server choose to apply on updates. Here's how it actually works in git, then how git-sync's choices map onto it.

A ref is just a name → hash mapping

On disk it's literally a file in .git/refs/ (or an entry in packed-refs) containing a hash. Nothing in that data says "I'm a branch, I must fast-forward." The ref data structure in refs.h has no FF concept at all.

FF is enforced at update time, not by the ref itself

There are two independent checks in upstream git, both policy:

• Client side, in remote.c:set_ref_status_for_push — when you run git push, the client computes whether each update is FF and refuses non-FF updates locally unless you pass --force.
• Server side, in receive-pack.c — controlled by receive.denynonfastforwards, and it only applies to refs/heads/*. Notes, pull refs, custom namespaces — the server happily accepts non-FF updates on those.

So in standard git, the rules are:

That last row is the important one. Notes get rewritten non-FF as part of normal usage. Git doesn't try to FF-check them because it would be wrong to.

What --mirror actually does

In builtin/push.c:568:

--mirror is literally two things combined:

1. MIRROR — include all refs in the push (not just ones with a local peer)
2. FORCE — bypass the FF policy on everything

That's it. There's no special per-namespace logic. Git just says "force everything, send it all, let the server's defaults sort it out."

So why doesn't git-sync's relay just do the same?

This is the design choice. Git-sync's sync mode promises a stricter contract than git push --mirror: it won't clobber a ref non-FF without you asking. The relay path is the embodiment of that contract. So when a RefKindOther ref shows up, relay refuses — not because it couldn't send the update, but because relay's contract says "I only do FF-safe updates, and I can't even check FF for these refs."

That's why the split exists:

• sync mode = git push (FF-safe contract). Won't relay other-kind refs.
• replicate mode = git push --mirror (overwrite contract). Will relay other-kind refs — and the comment at relay.go:166-168 says exactly this.
• sync --force is the awkward in-between: the planner conservatively still bails to materialized, even though, as you correctly point out, it could in principle just relay-and-overwrite the way --mirror does.

To answer your question directly

If a ref now points to a different commit, why not just update it on target?

You can. Git does this with --mirror. The only thing stopping git-sync's relay from doing the same for RefKindOther under --force is a planner policy decision, not a protocol or data-model constraint. The relay code path that would handle it already exists — it's CanReplicateRelay. It just isn't reused when you're in sync mode with --force.

If your use case is "I want git push --mirror semantics," replicate mode is the right tool. If you want sync-mode-with-force to behave like that too, that's a reasonable planner change — extend CanIncrementalRelay so force=true accepts RefKindOther plans the way replicate does.

The "additional work" is essentially: walk the full object graph locally and build a self-contained pack, instead of letting the source server do a smart negotiation for you. Here's why that's necessary for other-kind refs.

How relay gets away with sending almost nothing

Relay leans on git's thin pack negotiation:

1. Relay tells the source: "wants = [new tips], haves = [target's current refs]"
2. The source server walks reachable(wants) − reachable(haves) and sends only those objects
3. The resulting pack may even contain deltas against objects the source assumes target has
4. Relay forwards that pack to target, which can apply it because target really does have the base objects

The critical phrase is "target really does have the base objects." For a FF branch update, that's guaranteed by definition — the target's old tip is an ancestor of the source's new tip, so everything the source elides as "covered by haves" is genuinely on target.

Why the same trick breaks down for other-kind refs

For refs/notes/*, refs/pull/*, custom namespaces, the new tip has no ancestry relationship with target's current refs:

• A refs/notes/commits rewrite produces a commit that's not derived from the old one
• A refs/pull/N/head ref points into a fork's history that target may or may not share
• A custom ref can be literally anything

The relay still could send target's refs as haves and ask the source to negotiate. The source would dutifully compute reachable(new_notes_tip) − reachable(target's refs) and send something. But:

1. The planner can't verify upfront that the result will be sound — there's no FF invariant to lean on
2. If the source produces deltas against objects it assumes target has, but those objects are only on target via some non-obvious path (or aren't there at all), the pack fails to apply
3. Tag-creates only work in relay because the tag's target commit is presumed reachable through some existing branch have. For an arbitrary refs/notes/commits create, there's no such presumption

The planner's choice is: rather than reason case-by-case about whether negotiation will be sound, declare the entire shape ineligible.

What materialized does instead

From internal/strategy/materialized/materialized.go:49-105:

1. Fetch objects into a local in-memory store (still using haves to minimize transfer)
2. Walk the full closure locally — compute every object reachable from the new ref tips
3. Subtract the closure of target's existing refs (ObjectsToPush)
4. Build a non-thin, self-contained packfile from the remaining objects
5. Push that pack to target

The key difference: materialized doesn't trust the source's negotiation to produce something target can apply. It does the walk itself, locally, with both sides' ref state in hand, and emits a pack that's correct by construction.

So the "additional work" is

• Materializing objects into a local store (extra disk/memory cost)
• Walking the source graph from new tips (CPU cost, proportional to graph size)
• Walking the target graph from its existing refs to subtract (more CPU)
• Repacking from scratch instead of forwarding the source's negotiated pack (no delta reuse from the source's pack)

For a small repo this is negligible. For a large monorepo with --all-refs, it can be a lot of object enumeration — which is why relay exists as the fast path for the cases where it's safe, and materialized exists as the correct-but-expensive fallback for everything else.

The deeper architectural point: relay outsources correctness to the source server's negotiator. Materialized takes correctness back into git-sync's process by computing the closure itself. Other-kind refs are the case where outsourcing correctness no longer works.

Honestly — I don't have concrete context on the specific HTTP timeout you're hitting. The earlier framing in this conversation talked about "materialized push over HTTP is broken" as a given, but I haven't seen the actual symptom: where it times out, what the logs look like, whether it's the source fetch or the target push, etc.

Before recommending an approach, I'd want to know:

1. Where does the timeout happen? — fetching from source, pushing to target, or during the local closure walk in materialized?
2. What's the scale? — is --all-refs pulling in a huge ref graph, or is it timing out even on a small repo?
3. What's the actual error? — connection idle timeout, request deadline exceeded, server-side timeout, body read timeout?
4. Is it reproducible? — same failure every time, or intermittent?

Without that, any "best approach" is just guessing between plausible fixes. The reasonable buckets are:

• A. Fix the materialized HTTP path directly — e.g., streaming the pack body, raising/removing a deadline, splitting one giant push into multiple, adding keepalive/heartbeat on the connection
• B. Avoid materialized for this case — extend CanIncrementalRelay so force=true accepts RefKindOther like replicate does, so --all-refs --force stays on relay
• C. Use replicate mode if the user's intent is mirror semantics — sidesteps materialized entirely for this scenario
• D. Switch transport — use SSH instead of HTTP for materialized pushes if the timeout is HTTP-stack-specific (your branch is soph/ssh-transport, so this may already be in play)

(A) is the right fix if materialized-over-HTTP is genuinely broken. (B) is the right fix if the real complaint is "this case shouldn't have needed materialized in the first place." (C) and (D) are workarounds.

Can you paste the actual error or point me at the test/log that shows the timeout? Then I can give you a real recommendation instead of a menu.

I read the actual code. The diagnosis is mostly right, but the reasoning has a gap and the fix needs adjustment.

What the code confirms

• push.go:221 (the review says 208, off by a bit) — yes, io.Pipe + goroutine encoder
• push.go:233 — sendReceivePack(ctx, conn, req, pr, ...) passes the pipe reader straight to the POST
• smarthttp.go:254 — PostRPCStreamBody calls http.NewRequestWithContext(ctx, POST, url, body) with the bare io.Reader

When Go's http client gets a body that isn't a recognized buffered type (*bytes.Buffer, *bytes.Reader, *strings.Reader) and req.ContentLength isn't set, it uses Transfer-Encoding: chunked. So yes, the request goes out chunked, with no Content-Length.

Where the review's reasoning is weaker than it sounds

Two things:

1. Relay (PushPack) uses the exact same code path — it also calls sendReceivePack with a streaming body (push.go:265). If chunked-encoded receive-pack requests genuinely broke this backend, relay would fail too. So the difference is unlikely to be "streaming vs. fixed-length" alone — it's more likely size, timing, or both. Materialized produces a much bigger pack (full closure) and the encoder pauses for store I/O while encoding, which can starve the HTTP body and trip a server-side idle timeout. That's a different failure mode than "no Content-Length."

2. "Backend reads first few KB then stops draining" — this is stated as fact but isn't established in the code. If that's from a real observation (server logs, tcpdump), great. If it's a hypothesis, the fix may not address the real cause.

So the diagnosis as written conflates two things: "no Content-Length" and "body stalls during encoding." Buffering to a temp file happens to fix both, which is why it's tempting, but they're separate problems with separate cleaner fixes.

On the fix itself

Buffering to a temp file works and is conservative. Concerns:

• It's not strictly necessary to use a file. Go will switch off chunked encoding the moment you set req.ContentLength AND provide a body. If the bug is purely Content-Length, you can spool to a *bytes.Buffer for small packs and only fall back to a file for big ones. But materialized is bounded by DefaultMaxMaterializedObjects = 500_000, so packs can be GBs — file is the safer default.
• Latency cost. Encoding fully before sending means a long silence before the first byte hits the wire. For very large pushes that's a worse UX than the current pipe (which at least starts streaming immediately). If progress reporting matters, factor that in.
• It doesn't fix relay if relay is also affected by the same backend bug at scale — the review acknowledges this. Fine to defer, but worth knowing.
• Cleanup. Temp file cleanup needs to be airtight across cancel/error paths. defer os.Remove after creating in os.CreateTemp is the obvious pattern, but the encoder error path needs to not leak.

Where I'd push back before writing code

Before implementing the fix, I'd want a one-line answer to each of these:

1. What does the server actually log/return when it fails? "Stops draining" is doing a lot of work in the diagnosis. If it's a 408/504/connection-reset, that points at a timeout. If it's a 411 Length Required, that points at Content-Length. Different problem, possibly different fix.
2. Does the failure correlate with pack size? If a small materialized push works and a large one doesn't, this is a timing/size problem, not a streaming-vs-fixed-length problem.
3. Does relay fail at similar scale? If you can force relay to produce a similarly large pack (bootstrap from empty?), does it also time out?

If (1) shows "missing Content-Length," the temp-file approach is correct. If (1) shows "idle timeout during upload," the right fix is probably send keepalive frames in sideband-during-progress or chunked-but-with-server-aware framing, not buffer-then-send.

My recommendation

Don't implement step 1 yet. Confirm the diagnosis first — capture the actual failure mode (server log line, response code, or tcpdump showing where the connection dies). Twenty minutes of confirming the cause saves a fix that masks the wrong symptom.

If the diagnosis holds up:

• Implementing the temp-file spool is the right narrow fix
• I'd also set req.ContentLength explicitly and let Go pick a non-chunked transfer
• Add a code comment naming the backend bug being worked around, so this doesn't become mystery code when someone revisits in six months
• Skip the "if conn is *HTTPConn" type-switch in PushObjects. That leaks transport knowledge upward. Push the buffering decision into the transport: HTTPConn.PostRPCStreamBody can spool internally when ContentLength is unknown. SSH stays untouched. Cleaner layering.

Want me to dig into the actual failure mode (run a materialized push against the target, capture the request/response)?

Good — paste the error / log line whenever you're ready, and I'll work backward from the actual symptom. While you grab it, a few things that'd help narrow it fast:

• the client-side error from git-sync (the message it prints when the run fails)
• any server-side log line from the entire backend at the same moment (response code, handler error, anything in entire.log)
• approximate pack size or object count for the failing run (if you have it — even "small repo" vs "monorepo-scale" helps)

Once I see the error I can tell you whether the chunked-encoding hypothesis holds or whether it's actually a timeout/size/draining issue, and we can fix the right thing.

This is genuinely useful. The diagnosis sharpens significantly with this data, and it also changes one thing I had wrong about the context.

Key observations from this output

1. The target is Cloudflare Artifacts, not the entire backend. That's a different problem framing — we're not fighting a bug in our own backend, we're fighting compatibility with a third-party git-over-HTTPS service that we don't control.
2. The error is use of closed network connection — Go's net package raises this when the remote side closed the TCP connection while the client was still mid-write. This isn't a Go timeout, isn't a context cancel, and isn't a 4xx/5xx response. The server hung up on us.
3. 7.86 KB @ 98 B/s transfer to target. Two things — the byte count is small (we barely started the body), and 98 B/s is the average, which means the connection stayed open for ~80 seconds while we only managed to send 7.86 KB. So it's not "instant reject." It's "server let us start, then closed during the upload."
4. GODEBUG=http2client=0 — you already disabled HTTP/2, which means HTTP/2-specific stream errors were the first thing that broke. Falling back to HTTP/1.1 just got us further before failing.

What this actually points at

The hypothesis "missing Content-Length" gets stronger, and here's why: git itself does not stream receive-pack with chunked encoding. Upstream git (http-push.c) buffers the entire request body to memory or a temp file specifically so it can send a real Content-Length. Cloudflare's git endpoint was almost certainly built and tested against actual git clients, which means it has likely never been exercised by a chunked-transfer-encoded receive-pack request.

The 7.86 KB / ~80s pattern is also consistent with Cloudflare's edge: it accepted the connection, started forwarding bytes to origin, and at some point an edge proxy with a chunked-upload size or idle limit closed the connection. The "first few KB then drop" symptom in the original review hypothesis matches.

But there's still ambiguity — it could be:

• (a) chunked encoding is rejected outright by Cloudflare's edge
• (b) Cloudflare allows chunked but has a body-size or time limit that 47 MB hits
• (c) some idle-timeout fires because the encoder pauses while reading from the source store

Cheap experiments to disambiguate

A few minutes each, ordered by information value:

1. Does git push --mirror from a local clone work to that same Cloudflare URL? If yes → Cloudflare works fine with the standard git client → our streaming/chunked path is the difference → fix is to match git's behavior (buffer to known length).
2. Does a tiny materialized push fail too? Do the same sync --all-refs against a one-commit test repo. If a 100KB pack fails the same way → it's an encoding/framing issue, not size. If it succeeds → it's a size or time threshold.
3. Capture the request headers we're sending. Run with GODEBUG=http2debug=1 removed and add httputil.DumpRequestOut (temporary). Confirm we're actually sending Transfer-Encoding: chunked with no Content-Length.

Want me to wire up (3) so we can see the actual request line + headers leaving the client? That's the cheapest definitive answer. Or, if you have a local clone of this repo and git push --mirror access to the Cloudflare endpoint, running (1) is even faster — a one-line shell command.

This detail changes the diagnosis significantly. Let me revise.

What we now know

• Run 1: --tags (empty target) → succeeded
• Run 2: --all-refs (target now populated) → failed

Both push to the same Cloudflare endpoint. So whatever fails isn't a property of the endpoint in general — it's something specific to the second run's flow.

Reading the throughput display more carefully

Both transfers show ✓. The 7.86 KB is the info/refs GET response from Cloudflare — it completed successfully. The 98 B/s is the average over total wall-clock time (~80s), not the actual GET speed. So the timeline is:

1. GET info/refs from Cloudflare → fast, completed early
2. Long gap — fetch 47 MB from GitHub, materialize objects locally, compute closure (this dominated the 80s)
3. POST git-receive-pack to Cloudflare → connection closed before any meaningful body went out

The error use of closed network connection is Go's net package saying: the transport tried to write to a socket that was already closed.

Revised hypothesis: stale idle connection in the HTTP keep-alive pool

After step 1, the HTTPS connection to Cloudflare went back into Go's connection pool. During step 2 (long gap), that connection sat idle. Cloudflare's edge — like most CDNs — closes idle TLS connections aggressively (commonly 5-60 seconds). By the time step 3 came around, the pool still held the connection but the remote end was already gone. Go's transport doesn't retry POSTs on this kind of failure (because the body may have been partially consumed), so the error surfaces directly.

Why this fits the --tags vs --all-refs split

In the bootstrap/relay path (which --tags against an empty target would have taken), the target POST happens almost immediately after the target GET — bytes flow through quickly because relay pipes source-to-target directly. No long idle gap, no stale connection.

In the materialized path, there's a multi-minute gap between target GET and target POST while the source fetch and local closure walk happen. That's exactly the window for a CDN to drop the idle TLS connection.

Why this kills the previous diagnosis

The proposed fix — spool to temp file, send fixed Content-Length — wouldn't help at all. The connection is dead before the first body byte goes out. Content-Length vs chunked encoding is irrelevant if Go can't even open the write.

Cheap experiments to confirm

Two minutes each:

1. Force fresh connection for the receive-pack POST. Set Transport.DisableKeepAlives = true (or set a short IdleConnTimeout) on the HTTP client used for target POSTs. If the second run now succeeds → diagnosis confirmed.
2. httptrace log on the failing request. Add a ClientTrace with GotConn and PutIdleConn callbacks. If we see "got reused conn" right before the failure → confirmed it's a pooled stale connection.

If confirmed, the fix is small

In internal/gitproto/smarthttp.go's HTTP client construction:

• either disable keepalive entirely for receive-pack POSTs
• or set IdleConnTimeout to something well under typical CDN idle limits (10-15s)
• or: detect the "use of closed network connection" error on the very first write and retry once with a fresh connection (works because no body bytes were consumed yet)

The retry-once approach is the most surgical — it handles this specific race without changing pool behavior for other requests. Go's transport already does this for idempotent methods; we'd be doing it manually for POST when we know the body is intact.

Want me to look at the HTTP transport setup and propose the exact patch, or run experiment (2) first to confirm before changing anything?

Found the relevant code. The setup is:

• smarthttp.go:138-151 — NewHTTPTransport returns http.DefaultTransport (the package-level shared one!) — which has IdleConnTimeout: 90s and aggressive connection pooling
• smarthttp.go:100-119 — HTTPConn holds an *http.Client built around that transport
• Both the info/refs GET and the receive-pack POST use the same client, so they share the pool

This is the setup that would produce exactly the stale-connection bug. Two ways to confirm — pick whichever you prefer.

Option A — definitive 1-line test (5 seconds)

Force a fresh connection for every request by editing smarthttp.go:138-151:

Then rerun the same sync --all-refs command. If it succeeds → stale-pool hypothesis confirmed, and the real fix is just a more targeted version of the same thing. If it still fails → my hypothesis is wrong and we need to look elsewhere.

This is destructive to performance (no connection reuse at all), so it's only for the experiment, not the final fix.

Option B — detailed httptrace (more info, slightly more work)

Add this near the top of PostRPCStreamBody in smarthttp.go (before c.HTTP.Do(req)):

(needs import "net/http/httptrace" and "os"). This logs every POST's connection state. If we see reused=true wasIdle=true idleFor=70s before the failure → confirmed exactly.

My suggestion

Run option A first. It's one line, gives a binary yes/no answer in one test run, and if it works the path to the real fix is clear:

• not DisableKeepAlives globally (kills perf for the common case)
• instead either:
  • set IdleConnTimeout to ~10s on this transport, OR
  • close idle conns explicitly before the receive-pack POST via transport.CloseIdleConnections(), OR
  • retry-once on use of closed network connection if zero body bytes have been written

If Option A doesn't fix it, then Option B (or both, plus packet capture) is the next step.

Want me to make the option-A edit now so you can rerun, or would you rather apply it yourself?

Yes, genuinely useful to keep. Connection-pool behavior is opaque, and "stale idle pool entry" failures look identical to lots of other transient HTTPS errors from the outside — without httptrace you're guessing. Reasons to keep it:

• Third-party HTTP targets are unpredictable. git-sync pushes to whatever the user points it at — Cloudflare, GitHub, Gitea, Forgejo, internal servers behind proxies. Each has different idle timeout and keepalive behavior. Future bugs in this space are likely.
• The cost is tiny. A ClientTrace is a struct of function pointers; if you don't attach it, zero overhead. If you do attach it, one log line per request.
• It pairs well with verbose mode. You already have a -v / verbose flag and a ProgressOut writer on HTTPConn. Gating the trace behind verbose means normal runs are silent, and -v users get connection-reuse info for free next time something looks weird.

Suggested permanent shape

Not a one-off debug print, but a small, always-wired trace that emits to the existing progress writer only when verbose:

(Adjust to whatever your verbose flag actually is — I'd need to check.)

If you want even more value, add a PutIdleConn callback too — that tells you when a connection went back to the pool. Pairing PutIdleConn (T0) with GotConn.IdleTime (T1) on the next request gives you a complete picture of pool lifecycle.

The narrow alternative

If you don't want it always-on but want it cheap-to-enable: gate it behind an env var like GITSYNC_HTTP_TRACE=1. Zero overhead until set, but instantly usable next time something is wrong. This is the approach I'd take if -v already produces a lot of output and you don't want to add to it.

Either way — yes, worth wiring in. Want me to add it (verbose-gated) once we confirm the fix works?

No response requested.

Diagnosis confirmed exactly. The key lines:

• Same socket (192.168.1.114:50582 ↔ 104.18.16.116:443) reused from the info/refs GET
• Idle for ~13s — Cloudflare's edge closed it during that window
• WroteRequest returned the error → no bytes reached the server, so this is a transparent connection-pool race, not a server-side rejection of any data

The previous "buffer to temp file for Content-Length" proposal would not have helped — the failure happens before any body byte goes out.

Fix options

Three real candidates, in order from cheapest to most robust:

Fix A — short IdleConnTimeout on the HTTP transport. One-line change in NewHTTPTransport. Set e.g. tc.IdleConnTimeout = 5 * time.Second so pooled connections expire client-side before any reasonable CDN closes them server-side. Trades a small loss of pool reuse for race-freedom in the long-gap case.

• ✅ trivial
• ⚠️ heuristic — if some endpoint closes in 4s we're back to the same bug

Fix B — explicit CloseIdleConnections() before the receive-pack POST. In PostRPCStreamBody, when service == "git-receive-pack", drop idle connections first. We know push always follows fetch work, so the connection is guaranteed stale-suspect.

• ✅ no heuristic, no buffering
• ⚠️ always costs one extra TLS handshake on the push
• ⚠️ doesn't help other long-gap call paths (none today, but a foot-gun for the future)

Fix C — buffer the pack body and set req.GetBody so Go's transport auto-retries. Spool the pack to a temp file, send with explicit ContentLength, point GetBody at a fresh reader. Go's http.Transport.shouldRetryRequest will then retry POST on pconnBroken / nothingWrittenError (which is what this is) because it knows the body can be replayed.

• ✅ most robust — handles any idle timeout, anywhere
• ✅ same code shape as upstream git (which also buffers receive-pack)
• ⚠️ disk space for the temp file (bounded by DefaultMaxMaterializedObjects = 500K, could be GBs)
• ⚠️ encoder must complete before send starts → slightly worse first-byte latency

My recommendation

Do A + B together:

• A as a defense-in-depth default — cheap, helps every code path
• B as the targeted fix for the known long-gap site

Skip C for now. It's correct but heavier, and the original motivation (Content-Length / chunked encoding) turned out to be a red herring. If A + B doesn't hold up against some other CDN, C is still on the table.

Both A and B are ~5 lines of code. Want me to implement them?

You're right. For a CLI that's already pushing tens of MB to multiple GB over a single connection, an extra TLS handshake at the start of a push is a rounding error — maybe 100-300ms against transfers measured in seconds-to-minutes.

If handshake cost doesn't matter, then Fix A is unnecessary and Fix B is overkill in scope. The simpler and more honest choice is:

Just disable keep-alives entirely on the HTTP transport.

Concretely, in NewHTTPTransport:

What this trades

• Loses: connection pooling between info/refs GET and the immediately-following upload-pack / receive-pack POST on the same host. That's the one place pooling actually buys us anything in this workflow — and it's a single saved TLS handshake (~100-300ms).
• Gains: no possible stale-pool race against any CDN, no heuristic timeout to tune, no per-call CloseIdleConnections() sprinkled around, no temp-file buffering. Just always-fresh connections.
• Cost per sync: ~3-5 extra TLS handshakes total (source info/refs, source upload-pack, target info/refs, target receive-pack, maybe v1 fallback probe).

For a CLI whose typical run is dominated by tens of MB to GB of transfer over a single TCP connection per phase, that cost is genuinely invisible.

One thing worth being explicit about

This is a deliberate trade — "we're a one-shot CLI, treat each HTTP request as standalone." Worth a code comment so a future contributor doesn't see DisableKeepAlives = true and assume it was a debugging leftover. Something like:

Want me to apply this? It replaces the whole A+B proposal with a one-liner.

That's a clean framing. "New connection per step" lines up with what git-sync actually does over HTTP:

Each step is one HTTP request. There's no tight back-to-back request pattern where pooling actually buys us anything. And between steps there's real work (planning, local materialization, closure walking). So "fresh connection per request" matches the actual workflow rather than fighting it.

Library angle

This is solvable cleanly with the API already there:

• NewHTTPTransport returns the default git-sync-tuned transport — keep-alives off, "new connection per step" semantics
• NewHTTPConn(ep, label, auth, rt http.RoundTripper) accepts any RoundTripper, so library users who want pooling (e.g., embedding git-sync in a long-running server that does many syncs against the same host) can pass their own transport with whatever pool config they want

That way:

• CLI gets the safe default for free
• Library users get the same default
• Library users with a different workflow can opt out by passing their own transport

One thing worth fixing while we're here

The current NewHTTPTransport(false) returns http.DefaultTransport — the package-level shared one. That means our settings would leak into any other code in the same process using http.DefaultTransport. For a library that's a latent footgun. Whatever we change, we should also switch to always returning a Clone() so our config is isolated.

Proposed concrete change

Two behavior changes from current code: (1) always clone, never return shared default; (2) keep-alives off.

Want me to apply it?

Done. Branch soph/http-stale-pool-fix has two commits:

1. d8a071d9 — gitproto: add GITSYNC_HTTP_TRACE env var for connection diagnostics — the httptrace instrumentation we used to confirm the diagnosis, kept as a permanent debugging tool gated by env var.
2. a3a96da — gitproto: disable HTTP keep-alives on the default transport — the fix: always clone, never share http.DefaultTransport; set DisableKeepAlives = true.

All tests pass. The unrelated .entire/.gitignore change and .clawpatch/ are untouched in the working tree.

Recommended next step: rerun the original failing command (sync -v --all-refs ... against Cloudflare Artifacts) on this branch to confirm the bug is gone in practice. You can also rerun with GITSYNC_HTTP_TRACE=1 to confirm you now see fresh connections (reused=false) on the receive-pack POST.

The fix worked for what it addressed — reused=false, fresh TCP connection, fresh TLS handshake — but the failure persists at almost exactly the same byte count (~7.90 KB). So my diagnosis was incomplete: stale-pool was a real bug, but it wasn't the bug causing this particular failure.

What the new trace tells us

• Connection is genuinely fresh (reused=false, new local port 51475, even a new remote IP 104.18.17.116)
• TLS handshake succeeds end-to-end
• WroteRequest err=use of closed network connection — Cloudflare accepts the TLS connection, lets us start writing the request, then closes
• Failure point is reproducible at ~7.9 KB — same in both runs, with and without our fix

That byte-count consistency is the new smoking gun. If it were timing or pool state, the failure point would vary. A fixed cutoff says Cloudflare's edge is rejecting something content-shaped, not state-shaped.

Revised hypothesis — the original review was right after all

Most likely: Cloudflare's git endpoint rejects chunked-transfer-encoded POST bodies. Upstream git always buffers receive-pack to compute a real Content-Length, so Cloudflare has likely never been exercised by a chunked receive-pack upload. The ~7.9 KB is what gets buffered into the kernel/TLS layer before the rejection propagates back as a closed connection.

This is exactly the path the original review proposed: buffer the pack body, send with explicit Content-Length. I should have weighted that hypothesis more carefully when the pool-stale fit started forming.

Two things to do, in order

1. Confirm: dump the outgoing request headers once to verify we are actually sending Transfer-Encoding: chunked with no Content-Length. One-line change with httputil.DumpRequestOut gated by the same env var. Two minutes of work, eliminates ambiguity.

2. Fix: implement the temp-file buffering in PushObjects (or, cleaner, push the buffering down into HTTPConn.PostRPCStreamBody so SSH transport stays untouched). Send with real Content-Length, set req.GetBody to make the body replayable. Done.

The stale-pool commit stays — it was a real bug and the trace data confirmed it was happening. It just turned out not to be the bug for the Cloudflare case. Keep it as defense in depth.

Want me to do step 1 first (confirm headers), or go straight to step 2 (implement and test)?

Now rerun your same command. The output will include an [httptrace] POST git-receive-pack outgoing request: block before the failure with the actual headers Go's transport produced. What I'm looking for:

• Transfer-Encoding: chunked → confirms chunked encoding, hypothesis holds, proceed to temp-file fix
• Content-Length: <number> → chunked is not the cause, we need a different angle
• Expect: 100-continue → another possible Cloudflare-incompatibility worth ruling in/out

Paste the dump block (the Authorization header is redacted) plus the failure line and I'll tell you what we're looking at.

Branch state — four commits, each surgical:

1. d8a071d9 — httptrace env var (debugging tool)
2. a3a96da — disable keep-alives (the stale-pool defense)
3. 663e1ae — outgoing request dump (the diagnostic that confirmed chunked encoding)
4. 77ccb30e — spool unknown-length bodies for Content-Length (the actual Cloudflare fix)

Now rerun the original failing command. Expected outcome:

• the request dump should now show Content-Length: <number> and no Transfer-Encoding: chunked
• the push should complete

If you want to keep the trace output verbose for confirmation, run with GITSYNC_HTTP_TRACE=1. If you'd rather see clean output, drop the env var. Paste back what happens.

Yes — and you're right to push on this. Let me be honest about what the current fix sacrifices vs. what it should sacrifice.

The architectural picture

git-sync has two fundamentally different push shapes:

The "git-sync as a streaming proxy" property applies to relay, not to materialized.

What my fix actually changed

My current fix spools in PostRPCStreamBody, the transport layer. That's too broad — it affects both push shapes:

• Materialized: was never end-to-end streaming. Spooling on upload doesn't change its fundamental shape (full-fetch → encode → push). Loss: marginal.
• Relay: was end-to-end streaming. Spooling forces source bytes onto local disk before they leave for the target. Loss: real. We pay 2× disk I/O and lose the in-flight nature of the proxy.

So yes — for the relay paths, the current fix breaks the streaming property unnecessarily. The Cloudflare bug only ever manifested in the materialized path (your data: --tags against empty target → bootstrap-relay → chunked encoding → worked; --all-refs → materialized → chunked encoding → failed).

The right scope

Narrow the fix to materialized only:

• Move the spool out of PostRPCStreamBody and into the materialized push code (or have PushObjects produce a *spooledBody ahead of time)
• PostRPCStreamBody then just honors req.ContentLength / req.GetBody when the caller hands it a *spooledBody, and passes streaming readers through unchanged
• PushPack (relay) keeps streaming with chunked encoding, the way it does today and the way your --tags run proved works against Cloudflare

This preserves:

• The "streaming proxy" identity for relay
• The Cloudflare workaround for materialized (which was never streaming anyway)
• Disk usage proportional to the pack only when we're materializing — i.e., already buying a local-store roundtrip

The trade-off we accept by narrowing

If a future user with a really large repo hits a chunked-encoding-rejecting server via relay (large bootstrap, say), they'd see the same use of closed network connection. We don't have evidence this is happening yet — and the symmetry is fixable later by extending spooling to relay too if we ever see it. Today, narrowing is the right scope.

To do this

The branch hasn't been pushed yet (you confirmed by running locally), so I can either:

• Amend the existing spool commit to be narrower (rewrite history, cleaner final PR)
• Add a 5th commit that narrows the previous fix (preserves the diagnostic story in history)

I'd suggest amending since the branch is local-only and a 5th commit that says "actually I went too broad on commit 4" makes for a noisy PR. Want me to amend?

Done. Branch state is now:

1. d8a071d9 — httptrace env var
2. a3a96da — disable HTTP keep-alives
3. 663e1ae — request dump under trace
4. 2ff89c05 — spool materialized push body for explicit Content-Length (narrowed)

The architectural property is preserved: relay paths (PushPack) still stream source bytes straight through to target with chunked encoding. Only the materialized path — which already required a local store, so was never end-to-end streaming — buffers the body for the Cloudflare-compatibility win.

Rerun the original sync --all-refs command when you're ready. Expected outcome: it succeeds, and if you run with GITSYNC_HTTP_TRACE=1 you'll see Content-Length: <bytes> on the receive-pack POST (no Transfer-Encoding: chunked).

You've caught a real gap in the diagnosis. Let me be honest about what we actually know vs. what I assumed.

The contradiction

If chunked encoding alone were the issue, your --tags run would have failed too. It used bootstrap-relay, which also sends chunked. But it worked. So "Cloudflare rejects chunked" can't be the complete story.

What we actually know

• --tags (bootstrap-relay, chunked, ~smaller pack against empty target) → worked
• --all-refs (materialized, chunked, larger payload) → failed
• The failure point is ~7.9 KB consistently across runs
• The wire format dump confirmed Transfer-Encoding: chunked on the failing request

The differences between the two runs aren't just chunked-vs-not. They're also:

1. Strategy — bootstrap-relay forwards source's pack bytes as a continuous stream; materialized produces pack bytes from a local store with pauses for store I/O
2. Total payload size — --all-refs brings in many more refs (PRs, notes) and a different pack shape
3. Update-request header size — --all-refs against a populated target sends many more ref-update pkt-lines than --tags against an empty target

Likely real cause (revised)

CDN-fronted services like Cloudflare commonly impose conservative request body limits when there's no Content-Length — because without an upfront length, the edge can't plan buffer allocation, and the default ceiling is small. With Content-Length, the edge knows what to expect and accepts much larger uploads. This is consistent with:

• Bootstrap-relay worked because the chunked payload was under whatever Cloudflare's no-length ceiling is
• Materialized failed because the chunked payload exceeded it

That's a hypothesis, not a fact. The "rejects chunked outright" framing in my previous explanation was wrong — it's more likely "rejects chunked uploads over some size threshold."

What this means for the fix

The fix is still defensible, but for a different reason than I gave:

• Sending Content-Length lets Cloudflare's edge allocate properly and lifts the no-length size ceiling
• It also matches upstream git's behavior (which has always buffered receive-pack bodies)
• And gives us GetBody-based retry as a bonus

But we don't have proof it works until you actually run the failing case against the patched binary. There's a chance the threshold is actually somewhere else (e.g., absolute request size regardless of length header, or some Cloudflare-Artifacts-specific behavior).

What you should do

Just run the original failing command on this branch:

Three possible outcomes:

• Success → the Content-Length hypothesis holds. The trace will show Content-Length: <bytes> on the POST, no Transfer-Encoding. We can write a release note that's honest about the trade-off.
• Still fails at ~7.9 KB → it's not Content-Length. The cause is something else (specific byte pattern, header size, Cloudflare quirk). We'd revert / rethink.
• Fails differently → new failure mode tells us something new.

The fix is small enough and well-justified enough on independent grounds (matching upstream git, retry support) that even if Cloudflare's bug is something else, this is still a sensible improvement to keep. But we should know which world we're in. Want to run it and paste the output?

Confirmed — Content-Length was the missing piece. Diagnosis validated:

• Cloudflare accepts the chunked relay request when small (bootstrap-relay --tags)
• Cloudflare rejects the chunked materialized request when payload exceeds its no-length ceiling
• With Content-Length set, Cloudflare allocates properly and accepts the upload

The "really long repack" is the unavoidable side of the trade: encoding the entire pack to a temp file before the first byte goes on the wire, instead of overlapping encode+upload via io.Pipe. Materialized was never end-to-end streaming, but the encode-while-upload overlap did provide some apparent progress. Now you see silent local work, then the upload.

A few loose ends worth raising

1. Repack-time UX. "Really long" with no output looks like a hang to anyone watching. Worth adding a one-line "encoding pack..." (or progress with object counter) before NewSpooledBody so the user knows what phase we're in. Small change, easy to do on this branch or in a follow-up.

2. Branch is ready to push. Four commits, each scoped:

   • httptrace env var (debugging tool, kept)
   • keep-alives off (defense in depth, real bug but not the cause of this failure)
   • request dump (debugging tool, kept)
   • materialized body spool (the actual fix)

3. Local tree still has the unrelated .entire/.gitignore modification and the untracked .clawpatch/. Both untouched, separate from this work.

What would you like next?

• Push the branch and open a PR
• Add encode-phase progress before pushing
• Both

Branch is now five commits. The new one (35d91950) lights up target: encoding pack: 12.3 MB, elapsed 18s in the transient progress row while encoding runs.

Try it with the same command and you should see the line update every 500ms instead of a silent wait, then a final encoding pack: 47.3 MB in 35s line before the upload starts. Without -v it stays quiet.

If you'd rather have the progress addition squashed into the materialized-spool commit (commit 4) for a cleaner PR, say the word and I'll amend.

Working as designed. Those two snapshots show the encoder's two phases exactly:

• 50s @ 6.3 KB — almost all of that time was the delta-selection phase. go-git's encoder walks the object graph and picks deltas before writing anything to the pack. The 6.3 KB is just the update-request header we wrote first.
• 1m10s @ 46.7 MB — once delta selection finished, the actual pack write happened in ~20 seconds. That's the rate-limiting factor for large pushes.

The elapsed time is what saves this from looking like a hang — the byte counter alone would have been misleading.

Branch is ready (five commits, all tests green, materialized push to Cloudflare confirmed working). Open the PR?

Done. With this you should now see, against the same Cloudflare scenario:

The "selecting deltas" line ticks while the encoder walks the object graph, then flips to "encoding pack" once pack bytes start flowing past the 12-byte header. Same elapsed counter throughout, so total time is honest.

Rerun and you should now see the phase transition visible in real time. Ready to push when you've confirmed it.

Sharp question. Let me trace it.

In the old code, the header was on the wire before delta selection started.

Old PushObjects flow:

1. req.Encode(&header) — write update-request to a bytes.Buffer (in memory)
2. body = io.MultiReader(headerBytes, pipeReader) — concatenate header bytes + pipe
3. POST starts, Go transport begins reading from body
4. Transport reads from bytes.Reader first → ~6 KB of header bytes flow to wire as chunks immediately
5. bytes.Reader exhausted, transport advances to pipeReader
6. pipeReader blocks — encoder goroutine is doing delta selection
7. After 50+ seconds, encoder finally writes pack header → pipe unblocks → next chunk goes to wire

So your intuition is right: the visible-on-the-wire pattern in the old code was "send header, then idle for 50+ seconds, then send pack bytes." That's a multi-tens-of-seconds write stall in the middle of an active HTTP request body.

Which means the diagnosis might actually be different from what I said.

A CDN edge like Cloudflare's has a request-body idle timeout for chunked uploads — if the client stops sending for too long, the edge assumes the client is gone and closes. The ~7.9 KB you saw transferred before failure matches roughly the update-request header size (≈ what your "encoding pack: 6.3 KB, elapsed 50s" later confirmed). So the real failure shape might be:

• header bytes flow → ✓
• delta-selection stall → Cloudflare's idle timer ticks → connection closed
• failure surfaces as "use of closed network connection"

The "Cloudflare rejects chunked encoding" framing in my commit message is probably wrong. The actual culprit is more likely: the encoder stalls the request body mid-chunked-stream, and Cloudflare won't wait that long. Bootstrap-relay worked with --tags because source pack bytes from GitHub flow steadily — no internal stall — so Cloudflare never saw an idle gap.

Does this change the fix?

No, the fix still works — spooling everything to a temp file means there's no mid-stream stall, the upload is one continuous burst, and Cloudflare is happy. Content-Length is a bonus but probably not the causal ingredient.

Should we update anything?

Two honest options:

1. Adjust the commit message for 2ff89c05 to reflect this — "spool to avoid mid-stream stall" rather than "spool for Content-Length to avoid chunked rejection." More accurate explanation for future readers. Cheap to do.

2. Verify which it is by sending the spooled body with chunked encoding (don't set Content-Length) against Cloudflare. If it works → confirms the stall hypothesis; if it fails → Content-Length is doing real work. A few-line experiment, runnable now.

3. Both — verify first, then update commit message based on result.

Want me to set up the verification experiment? Or just adjust the message based on the more plausible explanation?

Builds clean. Now run the same command, but with GITSYNC_FORCE_CHUNKED=1 set:

This sends the pre-spooled body with chunked encoding (no Content-Length). The request dump should now show Transfer-Encoding: chunked and no Content-Length.

Three outcomes and what they mean:

1. Succeeds → the mid-stream stall was the real cause. Cloudflare doesn't care about chunked vs. Content-Length; it cares about whether bytes flow continuously. The current fix works because spooling eliminates the stall, and Content-Length is incidental.

2. Fails at ~6-7 KB with use of closed network connection → Content-Length is doing real work for Cloudflare specifically. Spool + chunked isn't enough; the upfront length header matters.

3. Fails differently (later, different error) → some third factor, worth looking at the new symptom.

The change is uncommitted and clearly marked as an experiment — I'll revert it after we have the answer regardless of outcome.

Done. Branch state:

Commit 4fa3d817 now reflects the actual finding: the mid-stream stall during delta selection trips Cloudflare's idle-write timeout. The body code comments on SpooledBody, PostRPCStreamBody, and PushObjects are updated to match. Content-Length and GetBody are honestly described as side effects, not the cause.

Experimental GITSYNC_FORCE_CHUNKED toggle is gone. Tests still pass. Working tree back to its pre-experiment state.

Ready to push and open the PR when you are.

Done. Final branch state:

Summary of the review pass:

• 4 of 5 commits stand as-is on their own merits
• only 6f1f5944 (request dump) had a misleading framing — its commit message and doc comment said "diagnosing whether a server rejects chunked uploads," which is the hypothesis we falsified. Now it's framed generically as "see the wire format when a server behaves unexpectedly," which is what the tool actually does
• the two real bugs we found (stale pool + mid-stream stall) are each addressed by their own commit, both independently necessary
• the spool commit (f76e2308) accurately describes the stall as the cause and Content-Length / GetBody as side benefits

Tests still pass. Branch is honest about what was wrong and why each fix is there. Ready to push.

Behavioral diff between this branch and main, grouped by what a user would notice:

1. Materialized push against HTTP targets now works where it previously timed out

The headline fix. Specifically:

• HTTP receive-pack POSTs for the materialized strategy used to fail mid-upload against Cloudflare-Artifacts-class endpoints with use of closed network connection. They now succeed.
• Underlying change: materialized writes the full receive-pack body (update-request + pack) to a temp file before the POST, instead of streaming with io.Pipe. The POST then goes out with Content-Length and GetBody set, not Transfer-Encoding: chunked.
• Visible side effect: receive-pack POST starts after encoding finishes (previously it started immediately, then stalled during delta selection). For a large repo, "starting push" → "first byte on the wire" can be tens of seconds where before there was no perceptible gap.

2. Every HTTP request gets a fresh TCP+TLS connection

• NewHTTPTransport now returns a Clone of http.DefaultTransport with DisableKeepAlives = true, instead of returning the shared http.DefaultTransport.
• User-visible effect: ~3–5 extra TLS handshakes per sync (~100–300 ms each on a fast network). Eliminates the entire class of "stale pooled connection" failures against CDN edges that close idle TLS aggressively.
• Library callers wanting pool reuse can still pass their own RoundTripper to NewHTTPConn.
• Side benefit: our transport config no longer leaks into other code that uses http.DefaultTransport in the same process.

3. New diagnostic tooling (off by default, zero overhead until enabled)

GITSYNC_HTTP_TRACE=1 enables two new stderr outputs:

• Per-request connection lifecycle (GetConn, GotConn reused=..., ConnectStart/Done, TLSHandshake*, WroteRequest, PutIdleConn)
• The outgoing POST request line + headers (Authorization redacted) — what Go's transport actually puts on the wire

Useful for diagnosing connection-pool, TLS, or wire-format anomalies against unfamiliar HTTP targets.

4. New -v progress output during materialized push

Two new lines in the existing transient progress region:

Distinguishes delta selection (no bytes flowing) from pack writing (bytes flowing) — without this, the user sees what looks like a multi-minute hang. No output in non-verbose mode.

5. Things that are unchanged

Explicit non-changes worth being clear about:

• Relay paths (PushPack) still stream source bytes through to target with chunked encoding. The "streaming proxy" property is preserved for the relay shapes (bootstrap, incremental, replicate).
• SSH transport is untouched.
• Auth, ref discovery, planning, fetch behavior are all unchanged.
• Existing strategy decisions (relay vs. materialized) — unchanged. Materialized is still chosen in the same situations as before.

6. Operational requirements added

• Materialized push now needs ephemeral disk space proportional to the pack size in $TMPDIR (or the OS default temp dir). Capped indirectly via DefaultMaxMaterializedObjects = 500K objects. Files are cleaned up after the POST regardless of success/failure.

Net effect for end users: sync to Cloudflare-class HTTP targets via materialized strategy now works; everything else is the same or slightly more observable.

Sharp question — yes, this is genuinely new. Before this PR, materialized used io.Pipe between the pack encoder and the HTTP transport, so encoded pack bytes flowed through memory chunks without ever landing on disk:

No temp file existed because the pipe is synchronous and unbuffered — the encoder blocked until the HTTP transport read the next chunk. Peak memory was bounded by whatever Go's HTTP layer held in flight.

The fix changes that:

We had to do this because the original bug was the pipe stalling mid-stream during delta selection. The whole point of the fix is "encode all the bytes first, then send them in one burst." That requires somewhere to hold those bytes during the encode→send transition. Memory or disk are the two options; disk is the right choice because the pack can be hundreds of MB to multi-GB and we don't want to OOM the process.

So the resource trade is real:

• Before: ~0 extra memory, 0 extra disk, but the upload could stall and fail
• After: ~0 extra memory, +pack-size on disk during the POST, upload doesn't stall

The temp file lives in $TMPDIR (or OS default), gets cleaned up after the POST regardless of outcome, and is bounded by the materialized strategy's existing safety cap (DefaultMaxMaterializedObjects = 500K). In practice the file is the same size as the pack go-git produces — same content as what used to flow through the pipe, just now committed to disk first.

The honest framing in the release notes/PR description would be: "Materialized push now requires ephemeral disk space ≈ pack size in $TMPDIR. This is the cost of avoiding the mid-stream stall that was causing failures against Cloudflare-class targets."

If conserving disk really matters for some user, there are two future options worth knowing about (not in this PR):

1. Memory-tier the buffer — keep small packs in bytes.Buffer, only spill to disk above some threshold
2. Stream straight to disk on a separate goroutine and start the POST when the file is "big enough" — complex and not worth it unless someone hits a real disk-pressure case

Neither was justified to add right now.

Genuinely good question. Let me trace through whether it's achievable.

What "streaming" required in the old code

The old io.Pipe approach worked when:

• pack bytes flowed continuously from encoder → HTTP body → wire
• "continuously" meaning: no gaps long enough to trip a CDN's idle-write timer

That's the design contract relay/bootstrap satisfy naturally — source pack bytes arrive from upstream at network speed, no internal pauses.

Why materialized can't satisfy that contract today

The mid-stream stall isn't in our code — it's in go-git's Encoder.Encode. Look at its body:

Delta selection runs first, synchronously, and emits zero bytes — for tens of seconds on a big repo. Then writes happen. There's no public way to split these phases: selector is unexported, encode() (lowercase) is unexported. We can only call Encode() which does both.

So: as long as we use go-git's encoder, somewhere there will be a tens-of-seconds no-bytes period during pack construction. The only question is whether that period sits before the POST starts (current PR) or during the POST body (old code → the bug).

What could let us go back to streaming

Each of these would work, with their own cost:

1. Buffer in memory instead of on disk. Same temporal shape as the current PR (encode → send) but uses RAM ≈ pack size instead of disk. Trades one resource for a more constrained one. Bad for large monorepo pushes that already use lots of RAM for the in-memory object store. Easy to add as a memory tier (bytes.Buffer below threshold, temp file above).

2. Fork go-git's encoder so we can run ObjectsToPack separately. Then the slow phase happens locally before the POST, and encode(objects) streams through io.Pipe to HTTP as it did before. This gives true streaming with no extra disk or memory. Cost: maintaining a fork or upstreaming a patch — non-trivial.

3. Find an out-of-band way to keep the connection alive during the stall. Things considered and ruled out:

   • TCP keepalives — most CDN idle timeouts are application-layer (no body bytes for X seconds), not TCP-layer. Probably wouldn't help against Cloudflare specifically. Worth a 5-minute test if curious.
   • Sideband padding during the stall — git's smart-HTTP receive-pack body has no sideband channel from client to server; the body is updates + flush + pack with no slot for filler.
   • Sending the pack header proactively — pack header is exactly 12 specific bytes. Sending them in advance would confuse the server's pack parser.

My honest take

You could go back to streaming via (2), but it's a real fork-and-maintain commitment for a marginal win — the win being "no temp file in $TMPDIR." For a one-shot CLI that already requires a full in-memory object store for materialized, a ≤ pack-size temp file in $TMPDIR is a non-issue on virtually every system git-sync runs on.

The pragmatic middle path is (1) — small memory tier, large temp file:

• pack ≤ 64 MB → bytes.Buffer in RAM
• pack > 64 MB → temp file like today

That preserves the disk-free property for the vast majority of pushes while keeping the safety net for monorepo-scale ones. ~30 lines of code, no architectural change.

If you want me to add that memory tier to this PR I can. Otherwise I'd let this PR ship as-is and revisit only if someone files an issue about temp file usage.

Looking at the code, the reason is a mix of design choices, not hard requirements.

Why it's structured this way

The two-phase design buys go-git three things:

1. Parallel delta selection — delta_selector.go:64-75 groups objects by type and walks each group in its own goroutine (wg.Go(func() { dw.walk(objs, packWindow) })). Easy to parallelize when you have the full list; harder when you're streaming output.

2. Sorting before delta walking — dw.sort(otp) orders objects by size/type/recency before searching for delta bases. Sorting requires the full list in hand.

3. Clean separation — "decide what's in the pack" vs. "write the pack" is two well-defined operations. Easier to reason about, test, and refactor independently.

What the structural constraints actually require

Not as much as the current design suggests:

• Pack header count — must be numObjects. But len(objects) == len(hashes) (verified in delta_selector.go:88: the selector adds one ObjectToPack per input hash; it just decides which are deltas). So the header could be written before selection runs, without changing anything.
• OFS_DELTA backward references — deltas point backward via byte offset (entry() uses e.w.Offset()), so objects must be written in their final order. The order is determined by sort + selection, so writing has to wait for those.
• Pack footer — SHA1/SHA256 of everything written, computed incrementally. Doesn't need anything special.

The hard constraint is: selection has to finish before any object body is written. The soft constraint is: the current API doesn't let you separate the two.

Minimal upstream proposal

Split Encode into two public methods:

Three properties of this proposal:

• Fully backward compatible. Existing callers using Encode() see no change.
• Tiny. ~20 lines of code, two new exported methods, one rename of encode to EncodePrepared. No semantic change.
• Unlocks our use case. git-sync would call SelectObjects before opening the HTTP POST, then call EncodePrepared against the pipe. Pack bytes flow without interruption. Temp file goes away.

What the upstream PR would look like

Single commit: "packfile: expose SelectObjects / EncodePrepared so callers can stream output." Tests for the new methods (just call them sequentially and confirm output matches Encode()). Doc strings explaining when to use which.

The argument for accepting it:

• It solves a real interoperability problem (any caller pushing over HTTP to a CDN-fronted git endpoint hits this; we're unlikely to be the only ones)
• It exposes existing internal structure rather than introducing new mechanism — lowest risk to maintainers
• It does not commit go-git to maintain a streaming-selection encoder (the harder ask), just to expose the boundary that's already there
• It matches similar API splits in go-git elsewhere (Repository.Init vs Repository.Open, Storer.NewEncodedObject vs Storer.SetEncodedObject, etc.)

The harder ask (only mention if asked)

A truly streaming encoder — interleaving selection with output — is possible but requires:

• Bounded deltaIndex memory (currently builds the full delta map upfront)
• Serializing within a type group (loses the cross-type parallelism)
• Probably 200–400 lines of refactor
• Hard to get accepted unless someone wants to do the work and benchmark it

Not worth proposing as part of the same PR. The "expose the boundary" proposal is the easy win and unblocks our use case completely.

If you want, I can draft the upstream PR. A go-git PR plus a follow-up git-sync PR that switches from temp file to SelectObjects + streaming pipe. Two clean changes; the temp-file approach in this PR becomes the fallback for when callers are pinned to a go-git version that predates SelectObjects.

Here's something you can paste into a message:

Problem

packfile.Encoder.Encode runs delta selection (slow, zero output for tens of seconds on large repos) and pack writing (fast, streamed) in a single sequential call. Callers streaming the encoder's output to an HTTP request body — e.g. pushing to git-receive-pack over HTTP — can't separate these phases, so the request body stalls during selection. CDN-fronted git endpoints (we've seen this against Cloudflare's git frontend specifically) interpret that stall as an idle upload and close the connection mid-stream.

Proposed change

Split Encoder.Encode into two exported methods that just expose the existing internal phases, keeping Encode as a thin wrapper for backward compatibility:

Roughly 20 lines of code, no behavioral change, fully backward compatible. It just makes the existing phase boundary callable. Callers that need streaming output (like us, pushing to receive-pack over HTTP) can run SelectObjects ahead of opening the body, then stream EncodePrepared into the request. No internal restructuring, no new mechanism.

Happy to put up a PR if this is something you'd consider.

Adjust the tone to match how you usually message that maintainer. The substance is: tiny patch, clear motivation, no commitment from them beyond exposing existing internals.

Paulo's suggestion is actually cleaner than my proposal. Here's why and a draft reply:

Why his approach is better

My proposal split Encode into two new exported methods. Paulo's keeps the existing Encoder API untouched and instead:

1. Exports the delta selector so callers can drive it directly
2. Adds WithObjectSelector(x) as a functional option on the encoder, so callers can inject a precomputed-results selector

This is better because:

• It preserves the Encoder API surface exactly
• It opens the door to other useful customizations beyond our streaming case (alternate delta strategies, mocking for tests, custom heuristics)
• "Expose the selector + allow injection" is a more orthogonal change than "expose phase boundary"

It still solves our problem — we pre-run the selector before the HTTP body opens, then pass a passthrough selector to the encoder so Encode()'s internal selection step returns instantly and the write phase streams continuously.

The third point is genuinely exciting

Paulo's separate idea — Storer-driven streaming hand-off based on post-negotiation reachability — would obviate our workaround entirely. The materialized push problem we hit is exactly the producer-side streaming problem he's describing. Worth flagging that we'd be a tester.

Draft reply you can paste:

That works better than what I proposed — keeping the Encoder API untouched and making the selector the injection point is more orthogonal, and it opens up other useful customizations (alternate delta strategies, test mocks, etc.) beyond just our streaming case.

To make sure I'm following the intended flow:

The exports that would unblock us:

• DeltaSelector (constructor + ObjectsToPack)
• ObjectToPack (the type returned)
• ObjectSelector interface
• WithObjectSelector option

Happy to put up the PR if you'd like, or defer to you if you'd rather scope it.

On the Storer-driven streaming hand-off: very interested. We hit exactly that shape on receive-pack (pushing a materialized pack to a CDN-fronted HTTP target — Cloudflare in our case closes the connection if the body idles during delta selection). If the Storer can stream pack bytes directly to the transport, the producer-side workaround we currently have (spool the whole pack to a temp file before opening the POST) goes away. Glad to be an early tester / give feedback when you have something to look at.

Tweak the closing tone if you usually message Paulo more casually.

PR opened: https://github.com/go-git/go-git/pull/2142

Summary of what landed in the PR:

• 5 new public APIs in plumbing/format/packfile:
  • DeltaSelector (exported from previously unexported deltaSelector)
  • NewDeltaSelector
  • ObjectSelector interface
  • WithObjectSelector EncoderOption
  • PrecomputedSelector convenience function
• 1 signature change: NewEncoder now takes variadic EncoderOption values — existing 3-arg call sites unaffected
• 2 new tests covering both round-trip equivalence with the default path and the nil no-op
• Full suite passing, gofmt -l and go vet clean
• DCO-signed, Assisted-by: Claude Opus 4.7 trailer per AI_POLICY.md
• PR body references Paulo's Discord suggestion so reviewers have the context for why this shape over my original split-Encode proposal

Once this merges (or even before, if you want to vendor a fork temporarily), the follow-up change in git-sync is: drop the temp-file spool from PushObjects, replace with DeltaSelector + precomputed + streaming via io.Pipe. That follow-up PR would entirely remove the disk-usage requirement we added to git-sync in this branch.

Summary of the review pass:

Fixed (committed as f0bd0928):

1. Encapsulated SpooledBody.path access — added (*SpooledBody).applyTo(req) so PostRPCStreamBody no longer reaches into unexported size/path fields. Cleaner abstraction boundary.
2. Trimmed the Cloudflare/idle-write narrative that was duplicated across four doc comments. Story now lives once on SpooledBody (the type that captures the workaround); PushObjects and PostRPCStreamBody point there.
3. Fixed the countingWriter comment — now explains why the counter is atomic (concurrent read from the progress ticker) instead of restating what the type does.

Verified-and-skipped (false positive or out of scope):

• "Dead selector field in Encoder" — agent was wrong. entry() at plumbing/format/packfile/encoder.go:177 calls e.selector.restoreOriginal(o) for cyclic-chain recovery. Field is used; doc comment is accurate.
• humanizeBytes duplicates syncer/progress.go:formatBytes — real duplication, but unfixable in scope: syncer imports gitproto, so consolidation requires moving the helper down the dependency graph. Bigger refactor than the cleanup pass justifies.
• PrecomputedSelector could be removed — Paulo specifically suggested it as the ergonomic API; keep.

Verified clean:

• httptrace callbacks (different field shapes; not worth unifying)
• Goroutine lifecycle in startPackEncodeProgress (stop/done channels safe)
• atomic.Int64 justified
• DisableKeepAlives = true documented honestly
• No efficiency issues on default code paths

Tests still pass. Branch is now six commits — five feature commits plus one explicit cleanup commit. If you'd rather, this last commit can be folded into the spool commit before pushing the PR; let me know.

Here's what I found about git-sync's commit message conventions and how the 6 commits on the branch comply.

What "linting rules" exist for commits

There's no formal commitlint/commitizen config, and mise run lint lints Go code, not commit messages. The conventions are:

1. CONTRIBUTING.md (loose guidance): "Commit with clear, descriptive messages" — example Add feature: description of what you added. Not enforced.
2. De-facto pattern from history: <area>: <lowercase verb> <description> — e.g. gitproto: add SSH transport via per-RPC ssh exec, tests: add end-to-end SSH transport coverage. Subject under ~70 chars.
3. Entire-Checkpoint: trailer (CONTRIBUTING.md): "All commits should include Entire-Checkpoint trailers from your sessions. These are added automatically by the prepare-commit-msg hook when Entire is enabled."

The 6 commits on soph/http-stale-pool-fix

Findings

1. Two commits missing Entire-Checkpoint: trailer — 6f1f5944 and f76e2308. Both were amended during interactive rebase; the prepare-commit-msg hook didn't re-fire on git rebase --continue. CONTRIBUTING.md says these "should" be present.

2. One non-imperative subject — f0bd0928 gitproto: post-review cleanup. Borderline. Every other commit on the branch (and most of the repo's recent history) uses an imperative verb. Could be more descriptive too — "post-review cleanup" doesn't say what was cleaned up.

3. All other format checks pass: <area>: prefix consistent, subjects under 70 chars, lowercase verbs after the colon, no trailing periods.

Fixes I'd apply if you want

• Rebase-edit 6f1f5944 and f76e2308 to invoke the prepare-commit-msg hook (regenerates the trailer with the current session value, which is what the hook would do)
• Rebase-edit f0bd0928 to reword to e.g. gitproto: tighten SpooledBody encapsulation and trim docs — imperative and concretely describes the changes

Want me to do that pass?

Done. New subject: gitproto: tighten SpooledBody encapsulation and trim docs — imperative, concrete, 60 chars. The Entire-Checkpoint trailer is preserved. Branch state otherwise unchanged.

Pushed. PR #2142 now has two commits:

1. 711bd57b — original plumbing: format/packfile, allow injecting a custom ObjectSelector
2. b08ab5ba — plumbing: format/packfile, enforce single-use semantics on PrecomputedSelector

The fix:

• PrecomputedSelector now returns *precomputedSelector (pointer) carrying an atomic.Bool consumed flag
• First ObjectsToPack call consumes the selector and returns the slice; second call returns the new exported sentinel ErrPrecomputedSelectorReused
• Doc on PrecomputedSelector is updated to make the constraint explicit (with the why: Encode mutates Offset / WantWrite / cycle recovery)
• New regression test TestPrecomputedSelectorIsSingleUse asserts the second Encode call against a shared selector fails with the sentinel

The reviewer can respond on the PR; if they prefer the alternative (cloning per call) we can iterate. I'd also recommend you replace @<reviewer> in the commit message with the actual reviewer's handle before they see it — left as a placeholder since I don't know their GitHub username.

Pushed. PR #2142 now has three commits:

1. 711bd57b — original allow injecting a custom ObjectSelector
2. b08ab5ba — enforce single-use semantics on PrecomputedSelector (the silent-corruption fix)
3. dbe9d7e6 — address Copilot review on #2142 (the three review nits)

Review summary:

• selector vs objectSelector naming → renamed selector → deltaSelector. Encoder struct now has a doc comment explaining the two-selector split (one for object selection, one for write-phase recovery).
• PrecomputedSelector captured slice directly → defensive shallow copy at construction so callers can freely mutate their original slice (append/reslice). Element-immutability requirement explicitly documented alongside the existing single-use note.
• Test using suite buffer → switched TestWithObjectSelectorNilPreservesDefault to a fresh bytes.Buffer, matching the other precomputed-selector test.

All tests still pass. If the reviewer is happy with these, the PR can probably be squashed at merge time so the final history shows one tidy commit.

Paulo has three threads going. The first two are easy to address, the third one I think is based on a misread of what the API change actually unlocks. Here's a draft reply:

I'm not sure we need PrecomputedSelector in go-git, couldn't that be in git-sync and injected on encoder construction?

Agreed — PrecomputedSelector is a one-line passthrough wrapping []*ObjectToPack. Any consumer can write it themselves. Happy to drop it from this PR. Go-git's surface stays as: DeltaSelector (exported), ObjectSelector (interface), WithObjectSelector (option).

The fallback into the default makes this change awkward.

Could you say more about what's awkward? The current shape is: omit WithObjectSelector → encoder uses its internal DeltaSelector (existing behavior); pass WithObjectSelector(x) → use x. The fallback exists to keep NewEncoder(...) backward compatible. If you'd prefer something else — e.g. a separate constructor that requires a selector, or removing the default entirely and forcing all callers to pass one — I'm happy to restructure. What shape did you have in mind?

Wouldn't we get the same result if, DeltaSelector becomes publicly exposed (as per current PR), then we used it to then call the Encoder (as is) with a subset of the hashes returned from the DeltaSelector?

I want to double-check I'm reading this right, because I don't think the result is the same — and if it were, I'd happily drop the whole WithObjectSelector part too.

Encoder.Encode(hashes, packWindow) always calls selector.ObjectsToPack(hashes, packWindow) internally at encode time. So if a caller runs:

We end up doing selection twice. And the second run is what we needed to move off the wire — that's the streaming property the change is meant to unlock (for HTTP upload bodies that close on a mid-stream stall).

Without some way to hand the encoder a pre-built []*ObjectToPack and skip its internal selection step, the streaming property doesn't appear. WithObjectSelector is one way to provide that injection point. If you'd prefer a more explicit API — for example an EncodePrepared([]*ObjectToPack) method on Encoder so the selection-vs-write boundary is exposed bluntly without going through an interface — I'm happy to switch to that shape. Just let me know which feels right.

Three things to flag for you before you paste this:

1. The first two points are concessions — I'd drop PrecomputedSelector in the next iteration if he confirms
2. The third I'm pushing back on; if I'm wrong about Paulo's intent we should ask him to elaborate, not implement something that doesn't solve the problem
3. The EncodePrepared alternative I'm offering at the end is essentially my original proposal (before he suggested the selector-injection shape). If he prefers that shape over an option-based injection, we have a clean path; if he prefers the current shape minus PrecomputedSelector, we also have a clean path. Either way we converge.

Tweak the tone — I drafted it slightly more verbose than your usual register.

Pushed. PR #2142 now has four commits, with cfcf78c2 being the response to Paulo's feedback:

Removed from go-git:

• PrecomputedSelector constructor
• precomputedSelector private type
• ErrPrecomputedSelectorReused sentinel
• TestPrecomputedSelectorIsSingleUse
• unused sync/atomic import

Kept (unchanged):

• DeltaSelector / NewDeltaSelector exported
• ObjectSelector interface
• WithObjectSelector option
• TestWithObjectSelectorMatchesDefault — now uses a small local fixedSelector helper, demonstrating the passthrough pattern callers will write themselves

Net result for the PR: smaller surface area than before — go-git ships the injection point (WithObjectSelector + exported DeltaSelector); callers ship the precomputation strategy.

Implication for the git-sync side (when we follow up):

• We'll define a small precomputedSelector type in internal/gitproto (or wherever the materialized push lives)
• Single-use enforcement and defensive-copy semantics live there
• The git-sync change is otherwise identical to what we already drafted

Reply to Paulo could be as simple as: "Done — dropped PrecomputedSelector; the injection point + exported DeltaSelector are enough for any caller to assemble it themselves. Test updated to use a local passthrough so the precompute-then-stream pattern is still exercised."

Done. PR #2142 is now a single commit d254bade:

Net diff vs. main:

• plumbing/format/packfile/delta_selector.go — deltaSelector exported as DeltaSelector
• plumbing/format/packfile/encoder.go — ObjectSelector interface, EncoderOption, WithObjectSelector, variadic NewEncoder
• Tests for the precomputed-selector path matching default output, plus the nil-option no-op

Commit trailers:

• Signed-off-by: Stefan Haubold <stefan@haubi.com> (DCO)
• Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com> (preserved his contribution from 7de41efc)
• Assisted-by: Claude Opus 4.7 (per AI_POLICY.md)

Used --force-with-lease rather than --force so the push would have failed if anyone else (e.g. Paulo) had pushed something in the meantime. Stale PrecomputedSelector references in two doc comments got cleaned up as part of the squash.

Yes — the merge unblocks a clean follow-up. Here's what the work looks like and a couple of decisions worth making first.

What the follow-up does

Now that WithObjectSelector + exported DeltaSelector are in upstream go-git, materialized push in git-sync can move the slow phase (delta selection) out of the HTTP body entirely:

1. Bump go-git dep to a version that contains the merge
2. In PushObjects: define a local passthrough satisfying ObjectSelector, run DeltaSelector.ObjectsToPack ahead of time, then call Encoder.Encode behind that passthrough. Stream via io.Pipe to the HTTP body again (chunked, like relay).
3. Remove the SpooledBody machinery — no longer needed
4. Encode progress still useful, but now only shows the write phase (selection happens upfront and surfaces as its own line if we want)
5. Test against Cloudflare to confirm the streaming approach actually works in practice — we have circumstantial evidence it should, but no direct test yet

Tradeoffs we'd lose by switching

• Content-Length — chunked encoding returns
• req.GetBody retry — gone
• No temp file — pack stays in memory/streaming again, no $TMPDIR requirement

Both losses were nice-to-haves; the actual bug we solved was the mid-stream stall, and the new approach eliminates that at the source instead of working around it.

One open risk

Encoder.encode(objects) (the write phase) still calls entry() for each object, which reads from the storer and zlib-encodes. If a particular object is slow to read, that's a tiny mid-stream pause. In practice these should be sub-second, well under any CDN's idle-write timeout. But we should test against Cloudflare before declaring victory — same scenario that triggered the original bug.

Open question: how to land this on the branch

A couple of options for the branch history:

1. Add a new commit on top ("switch to go-git WithObjectSelector, drop spool"). Cleanest for review — the diff vs. the current branch tip shows exactly what changes. Keeps the iteration story.
2. Rewrite the branch so the final 5–6 commits reflect the final design without the spool→precomputed evolution showing. Cleaner final history but more invasive.

My recommendation: option 1 — the spool was the right thing to ship when we shipped it, the API change is what unlocks a cleaner approach, and that's an honest story in the PR.

Want me to proceed? If so:

• I'll bump go-git first and confirm the new API is reachable
• Then refactor PushObjects
• We'll test against Cloudflare together before pushing

Branch is now seven commits — the spool commits stay in history for the rollback story, the new one switches to the streaming API.

Before you push or merge, let's confirm the real-world test against Cloudflare. Same command as before, but on this commit:

What I expect:

• Progress shows: target: selecting deltas, elapsed Xs → target: selected N objects in Ys → target: encoding pack: Z MB, elapsed Ws → target: encoded pack: Z MB in Vs
• httptrace shows POST git-receive-pack outgoing request: with Transfer-Encoding: chunked (no Content-Length)
• POST succeeds end-to-end against Cloudflare Artifacts
• Total time is similar to the spool version (selection is still the dominant phase; we're just moving it before the POST instead of buffering)

If it succeeds → ship the branch. If it fails (and especially if it fails differently from before) → we have more data to work with.

Run when you're ready and paste the output.

The official mechanism is git credential reject — git's credential protocol has three verbs: fill (get one), approve (worked, keep it), reject (failed, forget it). Helpers that store credentials respond to reject by erasing the matching entry.

To unblock yourself right now, run:

That's the URL component of your target. Add username=... on its own line if you want to scope it to a specific account (some helpers store per-user entries). The next git credential fill will then either prompt or pick up whatever fresh token your helper is configured to return.

Why git-sync didn't surface this automatically

Looking at internal/auth/auth.go:69-72, git-sync calls git credential fill to obtain credentials but never calls git credential reject (or approve) afterwards. So when the helper hands us a stale token, we use it, hit 401, and surface the error — but the helper never learns the token is bad, so the next run gets the same stale token back.

This is missing-feature territory, not a bug in this branch. The fix is a small auth-side change:

1. Track which credentials came from git credential fill (vs. flags, env, Entire DB)
2. When a request returns 401 with credentials sourced from the helper, call git credential reject with the same protocol/host/username/password
3. Optionally call git credential approve on first success after a fill — convention, not strictly required for our use case

The reject input has the same shape as fill, but includes the username and password we used:

(blank line terminates)

Where to wire it in git-sync

The 401 surfaces in httpError (internal/gitproto/smarthttp.go). But the auth provenance lives up in internal/auth/auth.go. Cleanest plumbing:

• lookupGitCredential returns a func() error (the "reject closure") alongside the username/password, capturing the input it sent to fill
• The session-level code wraps the AuthMethod with that closure
• On 401, the session calls the closure (which runs git credential reject for the same input)

Worth doing? Yes — exactly because this scenario (expired token in helper, can't enter new one) recurs and is annoying. But it's separate from the stale-pool / streaming fix on this branch. I'd file it as its own change.

Want me to:

• A. Just answer the question; you'll handle the credential-reject feature later (immediate command above unblocks you now)
• B. Add the credential-reject support as a new branch off main (clean PR, separate from the streaming work)
• C. Add it to the current branch (couples two unrelated fixes — not recommended)

Nice. So --all-refs against Cloudflare now goes through end-to-end with the streaming approach — no temp file, chunked encoding, no stall. The whole branch story closes cleanly:

• httptrace diagnostics (kept)
• HTTP keep-alives off (kept — still needed for the stale-pool race)
• request dump under trace (kept)
• spool fix → switched to WithObjectSelector streaming (latest commit)
• pack-encode progress (kept, now split into selection-phase and write-phase lines)
• doc/encapsulation cleanups (kept)

Open items to think about before merging:

1. Push the branch and open the PR. Branch is local-only right now. Same gh pr create flow as before.
2. go-git pseudo-version in go.mod. Pinned to the merge commit. Once go-git cuts the next alpha (will be v6.0.0-alpha.5 based on their pattern), we can bump to that for a tagged dep.
3. Credential-reject feature — separate branch off main, not part of this PR. Want me to start it after this lands?

Want me to push the branch and open the PR now?

Done. Status of all three asks:

1. PR opened — https://github.com/entireio/git-sync/pull/64 — branch soph/http-stale-pool-fix with the full streaming-fix story (7 commits).

2. go-git version — pinned to the merge commit via pseudo-version (v6.0.0-alpha.4.0.20260521151600-590487407c38). No new alpha cut yet on go-git's side; will bump to a tagged release when one drops.

3. Credential-reject branch — soph/credential-reject-on-401 off latest main, one commit (c750c96b).

On the credential-reject branch:

• internal/auth — Resettable interface (optional, single Reset(ctx) error method). gitCredentialHelperAuth wraps the helper-sourced auth, captures the reject input, and shells out to git credential reject via the new GitCredentialRejectCommand hook (mirrors GitCredentialFillCommand for testability).
• internal/gitproto — maybeResetAuthOn401 type-asserts the conn's AuthMethod against the duck-typed resettable shape (no import cycle). Wired into both RequestInfoRefs and PostRPCStreamBody so 401s on either info-refs or upload-pack/receive-pack POSTs invalidate the cached credential.
• V1 scope — reset only, no auto-retry inside the same run. Rationale in the commit message: CI safety. Easy to add retry-and-prompt later if interactive UX becomes the priority.
• Tests — covering reject-input shape, the Reset shell-out, error wrapping, and that maybeResetAuthOn401 no-ops correctly on non-401 statuses, non-resettable auths, and nil.

Push & PR for this one whenever you're ready — git push -u origin soph/credential-reject-on-401 + gh pr create. I held off because it's a separate piece of work and I wanted you to read the design first.

Commit and push: