Fix RequestBody Read Error Handling

I checked both reports against current main. Both are real, and the fixes the issue proposes are sound. The issue is from Marcos Nils (Dagger), and he offered to open the push fix as a PR.

1. A push holds several full copies of the pack in memory — confirmed, and the issue only names one of the two affected paths

• On the v2 path, internal/remotehelper/githelper/push.go:129 reads all of send-pack's output into one bytes.Buffer. ReadSendPackRequest (gitproto/gitproto.go:317) allocates each payload and then copies it into that buffer, so every byte is held twice for a while.
• Then AppendAgentToReceivePackRequest (gitproto/agent.go:83) rebuilds the whole message in a second buffer, through replacePktLine, just to change the first pkt-line.
• All of this happens before X-Entire-Push-Size is sent, so the server can only refuse an oversized push after the client has already paid the memory.

Their fix works with our transport. doWithFailover (transport/proxy.go:431) only needs an io.ReadSeeker: it seeks to 0 before each attempt, for the 401 retry and for failover. An io.SectionReader over a list of chunks satisfies that. They're also right that we'd need to set ContentLength (and GetBody) by hand, because http.NewRequest only works these out for bytes and strings readers. Without them the request goes out chunked.

One edge case to check in their PR. Amending only the first chunk assumes the first command line, which carries the capabilities, sits entirely inside it. As far as I know git's send-pack.c, it splits the request into chunks of at most 65520 bytes (LARGE_PACKET_MAX). The first line is a few hundred bytes, so it fits in practice. Still, the code should return an error if the first line crosses the chunk boundary, not skip the amend silently.

What the issue misses: the v0/v1 connect path has the same problem. githelper/connect.go:48-63 does ReadReceivePackRequest, which builds one big buffer, then AppendAgentToReceivePackRequest, then bytes.NewReader. With protocol v2 Dagger never goes through it, but anyone on protocol.version=0/1 does. The chunking fix should cover both paths.

2. The replica cache keeps every repo it has ever seen and is rewritten on every operation — confirmed

• refreshReplicas (transport/proxy.go:618) calls replicas.Persist whenever the X-Entire-Replicas header is present. Persist runs discovery.ModifyCache, which takes the exclusive lock, reads, unmarshals, runs MarshalIndent, and writes the whole nodes.json atomically. This happens even when nothing changed.
• No code ever removes expired entries. GetRepoNodes reports them as stale, and the only caller, LoadFresh, ignores stale ones. So deleting expired entries on every write is safe: nothing reads stale entries.
• What the lock costs. Persist is best-effort, so a contended lock only adds delay: up to the 5s lockTimeout, then the write is skipped. It never makes the operation fail. Each Resolve also does an unlocked read of the whole file, so reads grow with file size too.
• Their two proposed fixes are right:
  • Delete expired entries in ModifyCache, or in SetRepoNodes.
  • Skip the write when the node set is unchanged and more than, say, half of the TTL remains. Without that margin, skipping writes would stop the 24h expiry from ever being renewed.

Suggested response

• Tell Marcos yes on the push PR, and ask for two additions:
  • Apply the chunking to connect.go as well.
  • Add a guard for a first line that isn't fully inside the first chunk.
• The cache change is small (prune and skip-unchanged in discovery/cache.go plus replicas.Persist). We can do it ourselves; it's maybe 40 lines with tests.

I haven't posted anything on the issue. I can draft the reply, or implement the cache fix in this worktree, whichever you prefer.

In the last paragraph of section 1, after the 220 MiB result:

The patch is about 120 lines touching push.go plus a new gitproto/chunks.go. Happy to open it as a PR if you'd take it.

So the offer covers only the push-memory fix. He doesn't offer anything for the replica cache. In section 2 he only proposes the two changes and describes Dagger's workaround: it clears the helper's cache directory when the engine starts, and deletes nodes.json once it passes 5 MiB.

The issue's header also says "Reply by Claude, reviewed by Marcos", so the whole text, including that offer, was drafted by an agent and then reviewed by him.

Both fixes are done on branch soph/remote-helper-push-memory-replica-cache. They're uncommitted; I haven't committed, pushed, or posted anything. Lint is clean, the Windows vet passes, and the affected packages' tests pass. I haven't run the full mise run check yet.

1. Push memory

Result: a 64 MiB push now allocates 65 MiB, down from 224 MiB on main. Heap reserved from the OS drops from 191 MiB to 71 MiB. I measured this with a throwaway test against a real git send-pack; it isn't kept in the tree.

What changed:

• New chunked body: a RequestBody type in internal/remotehelper/gitproto/body.go. It keeps the request in the pieces it was read in, rather than copying them into one growing buffer.
• Readers fill it directly:
  • ReadSendPackRequest keeps each send-pack packet as its own chunk.
  • ReadReceivePackRequest stores each command line as a chunk, then reads the pack in 64 KiB blocks.
• Agent amend copies only the first chunk: the new AppendAgentToReceivePackBody touches just the capability line. If that line doesn't fit inside the first chunk, it returns an error rather than skipping the amend silently.
• Retries still work: the transport gets a fresh reader over the chunks, so the 401 retry and replica failover can resend the body. ApplyTo sets ContentLength and GetBody, because net/http only works those out itself for bytes and strings readers.
• Both push paths are covered: push.go (protocol v2) and connect.go (v0/v1). The issue only mentioned the first. Both now share one receivePackHeaders helper for the size headers.

Test that couldn't catch a regression: TestHandlePush_RealSendPack_DeclaresSizeAndLands skipped its Content-Length check when the value was -1, which means a chunked upload. That is exactly how this change would break, so the test would have passed anyway. I made the check strict, then confirmed it fails when ApplyTo is removed.

2. Replica cache

• Expired entries are dropped on every write of nodes.json (discovery/cache.go), including clusters left with no repos. This is safe because nothing reads expired entries.
• Unchanged replica sets skip the write. replicas.Persist first reads the cache without taking the lock. It returns early if the same set is already stored (order doesn't matter) and at least half of the 24h TTL is left. Otherwise it takes the lock and rewrites as before. So a repo in steady use is rewritten about twice a day, not on every git operation.

Tests:

• pruning
• HasRepoNodes cases
• skipping an unchanged set
• refreshing an entry that's past half its TTL

I removed each change in turn and confirmed its test fails.

Still open: a push still holds the whole pack in memory once. Getting below that would mean streaming the upload, which gives up the declared-size header and the ability to resend the body. I left that out of scope.

Should I commit and open a PR? I'd link #2985, and run mise run check first.

No, it doesn't belong in go-git. Every buffer the fix removed is in our own code, and go-git isn't on the push path at all.

What we use from go-git here: only low-level parsing pieces.

• plumbing/format/pktline: ParseLength, LenSize, and the Flush/Delim/ResponseEnd constants, plus Scanner/ReadLine/PeekLine in a couple of places. These split the four-byte pkt-line length headers.
• plumbing/protocol/packp.SmartReply: strips the # service=... line from the start of an info/refs response.

What we don't use: go-git's transport, client, or any push code. A push in git-remote-entire works like this:

1. Real git send-pack --stateless-rpc (a subprocess) builds the request and the pack.
2. Our code in internal/remotehelper/gitproto unwraps send-pack's framing and adds our agent string to the capabilities. That's where the copies were: ReadSendPackRequest, ReadReceivePackRequest and AppendAgentToReceivePackRequest are all ours.
3. Our transport.Proxy sends the POST, handling replica failover, the 401 retry, and the size header.

The send-pack framing is part of git's remote-helper protocol, which go-git doesn't implement. The agent amend and X-Entire-Push-Size are specific to us, and the replica cache is entirely ours. Nothing upstream could take this fix.

go-git's own HTTP push has a similar problem. In the version we pin (v6.0.0-beta.1), plumbing/transport/http/handshake.go:545 writes the whole request into one bytes.Buffer (httpRequester) before posting it. That's a single growing buffer, so roughly 1–2× the pack. It doesn't affect git-remote-entire. It would only matter where we push through go-git's HTTP transport directly, and I haven't checked whether we do. If you want, I can check, and if so it's worth an upstream issue. It's separate from this fix either way.

No, it doesn't affect us. Nothing in the product sends a push or fetch over the network through go-git, so the go-git buffering never comes into play.

• Our pushes and fetches run the git binary. strategy calls remote.Push / remote.PushWithOptions / remote.Fetch, which are our own package cmd/entire/cli/checkpoint/remote, not go-git. PushWithOptions (checkpoint/remote/git.go:396) builds git push --no-verify --porcelain ... and runs it as a subprocess. Fetch goes through the same package.
• Pushes to entire:// remotes go through git → git-remote-entire → git send-pack, the path fixed on this branch.
• No production code imports go-git's network transport. The only non-test import of go-git/v6/plumbing/transport is none at all. The one test import, integration_test/http_remote_test.go, uses go-git's server-side backend to stand up a fake HTTPS remote. The client pushes to it with real git.
• The only go-git client clones and fetches are git.PlainClone and Fetch in checkpoint/checkpoint_test.go, against local directories, not HTTP.

So go-git is local-only for us: reading objects and refs, plus the pkt-line parsing in the remote helper. That fits the repo rule to use the git CLI for anything that writes. Whether to file the upstream issue is your call. It would help go-git users who push large packs over HTTP, but it has no effect on Entire.

The branch is still uncommitted. Should I run mise run check, commit, and open the PR linking #2985?

I fixed all three review points. Lint is clean and the affected packages' tests pass. The branch is still uncommitted.

MUST: ReadFrom treated a reader error as a clean end. The reviewer was right. io.ReadFull only knows io.EOF. So if the reader itself failed with io.ErrUnexpectedEOF, my code accepted the partial pack as complete. After a full 64 KiB block, the error was dropped entirely. The old io.Copy returned that error.

• The fix (gitproto/body.go): a plain read loop that fills each block, stops only on io.EOF, and returns every other error, even after a full block.
• The test: TestRequestBody_ReadFromPropagatesErrors makes the reader fail at 0 bytes, mid-block, exactly at a block boundary, and after a full block. It runs each position with io.ErrUnexpectedEOF and with an ordinary error. I put the old ReadFull logic back temporarily: the test failed in all four positions.

SHOULD: replay through the real transport. I added transport/body_replay_test.go. Both tests send a body of about 156 KiB, split into three chunks, through ServiceRPC. They check that every POST arriving at the server carries the full body and the right Content-Length.

• Failover: the first replica reads the whole body and returns 502. The second replica must receive all of it again, which only works if the failover rewinds the body.
• 307 redirect: net/http resends the body through GetBody. With GetBody removed, the redirect isn't followed and the test fails.

I didn't add a 401-retry test. It rewinds the body exactly as failover does, through the same build(), and it would need token-minting scaffolding.

SHOULD: the chunk-size comment was inaccurate. The comment on bodyChunkSize now says the block size is close to the largest pkt-line payload (65516 bytes), not equal to it.

Should I run mise run check, commit, and open the PR linking #2985?

I wouldn't make it a priority. If you want to contribute anyway, open an issue first rather than send a fix. Three reasons:

1. It doesn't affect us. Nothing in Entire pushes over HTTP through go-git. Any work there is an upstream contribution, not a fix for our users.

2. go-git's version is a smaller problem. Our fix mostly removed copies we made ourselves: a second full copy for the agent amend, plus payloads copied into a growing buffer. go-git has one growing bytes.Buffer in httpRequester. While that buffer grows, the old and new backing arrays exist together, so the peak is roughly 1.5–2× the pack, not 4×. I'm working that out from the code; I haven't measured go-git.

3. The worthwhile fix is a design change, not a patch. Swapping in a chunked buffer like ours would only remove the growth slack, down to about 1×. Getting below that means streaming. git's own remote-curl buffers requests up to http.postBuffer and switches to a chunked upload above it. In go-git, streaming collides with anything that has to resend the body: redirects and possibly reauth.go, which I haven't read. It also means sending without a Content-Length. Maintainers should weigh in on that before anyone writes code.

If you go ahead, my suggestion:

• Measure the peak RSS of a large go-git HTTP push first, so the issue has a number like Marcos's 797 MiB.
• File the issue with that number. Propose the postBuffer-style approach: buffer small requests, stream large ones, and send streamed ones without the resend guarantee.
• v6 is still in beta, so now is the cheapest time for them to change how this works.

I can write the measurement harness against v6.0.0-beta.1 if you want the number. Meanwhile our PR is still waiting on mise run check, a commit, and opening.

I made both review fixes. Lint is clean and the affected packages' tests pass. The branch is still uncommitted.

err == io.EOF instead of errors.Is. ReadFrom in gitproto/body.go now ends cleanly only on a bare io.EOF, as io.Copy does. A wrapped or joined error that contains EOF is now returned as a failure. errorlint already accepts this comparison on a value straight from Read, so it needs no suppression, only a one-line comment explaining why.

Tests for data and an error from the same Read. In body_test.go, TestRequestBody_ReadFromPropagatesErrors now runs every case two ways:

• the error on a Read of its own (as before)
• the error from the same Read that hands over the last bytes, using a small finalReadErr reader

Each way covers these sizes:

• 0 bytes
• a partial block
• exactly one full block, where the full block and the error come back from one call
• a full block and then more

The errors tried are io.ErrUnexpectedEOF, an ordinary error, and a wrapped io.EOF. A new TestRequestBody_ReadFromEOFWithFinalBytes checks the other side: a bare io.EOF returned with the final bytes, including a full block, is still a clean end and keeps all the data.

With errors.Is put back temporarily, the wrapped-EOF case fails in all 8 combinations, so the test guards the change.

Should I run mise run check, commit, and open the PR linking #2985?

The full check (mise run check) took longer than 10 minutes, so it's running in the background. I'll commit and create the trail once it finishes.

I opened trail 2710 for both fixes: https://entire.com/gh/entireio/-/trails/2710. The branch is pushed and linked to it as change cli/1745.

• Commit: dfff12de0b, one commit containing the push memory fix, the replica cache fix, and the review fixes. The message includes Fixes #2985.
• Description: what changed for each problem, the 224 → 65 MiB measurement, what the tests cover, and the check results.

mise run check: lint passed, and all tests passed except two. Both fail the same way on a clean main checkout on this machine, so this branch didn't cause them:

• TestHTTPS_PushFailsWithoutToken/git-refs hangs. It pushes with plain git to a test HTTPS server, so the code changed here isn't on its path.
• TestOpenCodeSeedRepoPlantsDeps fails on a missing node_modules symlink.

I committed despite those two, and the trail description says so. CI will show whether they also fail outside this machine.

Findings: none yet; the trail's review may not have run. I'll check again once it has, fix any real ones, and resolve them.

I haven't commented on #2985 or opened a GitHub PR.

I updated PR #2987: https://github.com/entireio/cli/pull/2987

• Title: "git-remote-entire: hold a pushed pack once; bound the replica cache", the same as the commit subject. It was just the branch name before.
• Body: Entire's placeholder text is replaced with the same description as trail 2710. It starts with Fixes #2985 and a link to the trail.
• Kept unchanged:
  • the <!-- entire-shadow-pr --> marker, which Entire needs to close the PR when the trail closes
  • the Cursor Bugbot summary block, which the bot rewrites on each review

The PR is still a draft; I haven't marked it ready for review.

I agreed with Copilot's comment and fixed it locally. Nothing is committed or pushed yet, and the review thread is still open.

The bug was mine. My agent amend only looked at the first chunk, but a shallow clone sends shallow <oid> lines ahead of the first command, which is where the capabilities sit. That broke two ways:

• v0/v1 connect: the agent string was silently never added, even with just one shallow line.
• v2 push: with about 1,200 or more shallow grafts, the command line crosses the first 65,515-byte chunk, and the push failed with an error. It used to succeed.

The fix (gitproto/body.go): the amend now walks the request line by line across chunks, skips lines without a NUL, and stops at the flush. It copies only the line carrying the capabilities and re-slices the chunks around it, so the pack is still never copied. This restores the old byte-level behavior without bringing back the extra copy.

Tests:

• gitproto:
  • the command line split at several points across chunks
  • shallow lines first, on the connect path
  • send-pack-style framing where the command straddles the 65,515-byte boundary
  • requests with no capability line, which are left unchanged
  • truncated or invalid lengths, which return an error
  • a check that the pack chunks are still the original memory
• The old first-chunk amend fails all the new shallow and split cases.
• New githelper/push_shallow_git_test.go: a real push from a shallow clone through git send-pack. It confirms that git really does send the shallow line first, and that the agent lands on the command. On v2 it doesn't catch the old bug by itself, because that needs over a thousand grafts; its comment says that coverage is in gitproto.

Lint, the Windows vet, and all remote-helper tests pass. I haven't rerun the full mise run check.

Should I commit, push, and resolve the Copilot thread? I'd resolve it without a reply, then check the trail findings after the push.

There's one open trail finding, 01M67JZSAW90 (High, body.go:128). It's the same bug as the Copilot comment, and no other PR threads are open.

The trail finding goes further than Copilot's comment: it cites git's own source and reproduced both failures against dfff12de:

• Connect path: a push from any shallow clone silently loses our agent suffix.
• v2 path: with about 1,235 or more grafts, the push aborts with pkt-line length N at offset 65455 exceeds message length.

The uncommitted fix already covers what it asks for:

• Both failures: the amend now reads lines across the whole request until it finds the line with the capabilities, so shallow lines first and lines split across chunks both work.
• The tests it asks for: a shallow line before the command on the connect path, and a command line that crosses send-pack's 65,515-byte boundary on v2. The new tests fail on the old code.
• The doc comment: updated as it suggests.

My fix differs from its suggestion in one way. It proposes merging the leading chunks into one buffer and amending that. I copy only the line with the capabilities and leave the surrounding chunks where they are. The pack is held once either way, so I'd keep mine. The finding also mentions saving one small allocation per command line on the connect path; that's negligible.

Should I commit and push? That would mean running mise run check (I expect the same two failures that main has), running lint, pushing, then resolving both the trail finding and the Copilot thread without a reply and re-checking the findings.

The full check is still running in the background; it's slow partly because one test that also hangs on main runs until it times out. I'll commit once it finishes.

I pushed the fix as 967b953e9d and resolved both review items without replying: the Copilot thread and trail finding 01M67JZSAW90.

• mise run check: the two tests that also fail on a clean main checkout failed again, TestHTTPS_PushFailsWithoutToken/git-refs (hangs until the 10-minute timeout) and TestOpenCodeSeedRepoPlantsDeps. Everything else passed, including all the remote-helper packages.
• Lint and Windows build check: lint found 0 issues on the committed tree, and GOOS=windows go vet passes.
• Trail findings: a re-check about a minute after the push showed none open. The trail's review of the new commit may not have finished yet, so I'll look again before calling it clear.

The PR is still a draft. Its description doesn't mention the shallow-clone fix yet; I can add a line if you want.

Both descriptions are updated. Next I'll commit the two comment fixes and push once the check finishes.

Only the OpenCode failure that also happens on main showed up. This time the HTTPS test didn't hang.