HTTP Connection Reuse and Probe Draining

Claude Code·Opus 4.8[1m]·Soph·3mo ago·58min·3 Checkpoints·4 file changes·+85/-9·45.5K tokens

can you look at this branch, and this specific change ask:

  1. Probe connections aren't drained, defeating the idle pool — repo_mirror_probe.go:211-231 (low/efficiency) probeClient is deliberately built with MaxIdleConns/IdleConnTimeout "to reuse the TLS session between ticks." But mirrorAdvertisesHead does defer resp.Body.Close() without draining, and reads via io.LimitReader. Go's transport only recycles a connection when the body is read to EOF before Close(); on the sr.Decode/adv.Decode error returns (and possibly the normal path, depending on whether go-git consumes the trailing flush-pkt) the body is left partially read, so the conn is closed instead of pooled — every 2s probe then pays a fresh TLS handshake. Add a drain before close: defer func() { _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, maxProbeBytes)) _ = resp.Body.Close() }() Over a 30-minute wait (~900 probes) that's the difference between reusing a connection and 900 handshakes.
3mo ago·6m

Done. The change is applied and lint passes.

Summary

The review finding was correct. In cmd/entire/cli/repo_mirror_probe.go (the file is at cmd/entire/cli/, not cmd/entire/cli/commands/), probeClient is deliberately built with MaxIdleConns: 2 / IdleConnTimeout: 90s to reuse the TLS session across the 2-second probe ticks. But Go's transport only returns a connection to the idle pool when the response body is read to EOF before Close(). mirrorAdvertisesHead was closing the body without draining it — and the sr.Decode / adv.Decode error returns leave it partially read — so the connection was discarded instead of pooled, forcing a fresh TLS handshake on every probe.

Fix (repo_mirror_probe.go:211): drain the body before closing, bounded by maxProbeBytes to match the existing read cap:

I used the codebase's existing //nolint:errcheck convention (matching template.go:147 and command_cancel_unix_test.go:60) because errcheck is configured with check-blank here. Build and mise run lint both pass.

One note: I didn't run the full mise run check (tests). This is a behavior-equivalent efficiency fix with no test covering connection reuse, but let me know if you'd like me to run the test suite or commit the change.

can you create a new branch off main and commit this? the PR merged already so we will do a new PR for this change

3mo ago·54.6s

Done. Created branch probe-drain-conn-reuse off origin/main (which already includes the merged keyring PR #1307) and committed the drain fix.

  • Branch: probe-drain-conn-reuse (tracking origin/main)
  • Commit: 4aed46f18 — mirror: drain probe body before close so the idle pool works
  • 1 file changed, 10 insertions(+), 1 deletion(-)

Note: main is checked out in your other worktree (/Users/soph/Work/entire/devenv/cli), so I branched directly off origin/main rather than checking out main here.

Want me to push it and open the PR?

4aed46fmirror: drain probe body before close so the idle pool works probeClient is built with a small idle pool and a 90s IdleConnTimeout specifically to reuse the TLS session across the 2s clone-readiness probes. But Go's transport only returns a connection to the idle pool when the response body is read to EOF before Close. mirrorAdvertisesHead closed the body without draining, and the sr.Decode/adv.Decode error returns leave it partially read, so the connection was discarded instead of pooled — every probe paid a fresh TLS handshake, defeating the pool. Drain the body (bounded by maxProbeBytes) before closing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: bf42c15ed2b6+10/-1

yes

3mo ago·28.8s

Pushed and PR opened: https://github.com/entireio/cli/pull/1320

  • Branch: probe-drain-conn-reuse → main
  • PR #1320: "mirror: drain probe body before close so the idle pool works"

/simplify → 4 cleanup agents in parallel → apply the fixes

You are improving the quality of the changed code, not hunting for bugs. Review it for reuse, simplification, efficiency, and altitude issues, then fix what you find. Do not look for correctness bugs — that is what /code-review is for.

Phase 0 — Gather the diff

Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1 if there's no upstream) to get the unified diff under review. If there are uncommitted changes, or the range diff is empty, also run git diff HEAD and include the working-tree changes in scope — the review often runs before the commit. If a PR number, branch name, or file path was passed as an argument, review that target instead. Treat this diff as the review scope.

Phase 1 — Review (4 cleanup agents in parallel)

Launch 4 independent review agents via the Agent tool, all in a single message so they run concurrently. Pass each agent the diff and one of the four angles below. Each returns its findings with file, line, a one-line summary, and the concrete cost (what is duplicated, wasted, or harder to maintain).

Reuse

Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.

Simplification

Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.

Efficiency

Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Name the cheaper alternative.

Altitude

Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.

Phase 2 — Apply the fixes

Wait for all four agents to complete, dedup findings that point at the same line or mechanism, and fix each remaining one directly. Skip any finding whose fix would change intended behavior, require changes well outside the reviewed diff, or that you judge to be a false positive — note the skip rather than arguing with it. Finish with a brief summary of what was fixed and what was skipped (or confirm the code was already clean).

3mo ago·3m

Confirmed — the change is internally consistent (drain bound matches the read bound on line 228) and correct.

Summary

Ran all four cleanup angles (reuse, simplification, efficiency, altitude) over the one-function diff. No fixes applied — the code is already clean.

  • Reuse: Clean. No existing drain-and-close helper exists in the repo; this is a one-off, so an inline drain is appropriate (other HTTP reads in the package either fully drain via io.ReadAll or use a fresh, non-pooled client).
  • Altitude: Clean. mirrorAdvertisesHead is the only hot-path probe loop relying on a configured idle pool — every other HTTP read in the package either drains fully through api.DecodeJSON/io.ReadAll or doesn't pool connections. A local fix is correctly scoped, not a bandaid on shared infra.
  • Simplification (skipped): The flagged "verbose comment" matches this file's deliberately high comment density (CLAUDE.md instructs matching surrounding style) and documents a non-obvious Go transport behavior worth keeping.
  • Efficiency (skipped): The "drains 8 MiB on error paths" finding misreads io.Copy — it stops at the body's actual EOF, so small error bodies cost only their real size; the 8 MiB is a safety ceiling, and reusing maxProbeBytes keeps it consistent with the read cap below.

No changes to commit.

_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, maxProbeBytes))I'm not sure this has the intended result. [9:38 AM]Partial draining is the same as not draining. We probably want to: _, _ = io.Copy(io.Discard, resp.Body)

is this the same direction ?

3mo ago·2m

Yes — same direction, and you spotted a real flaw in my first version. Done.

Why your instinct was right: a LimitReader-capped drain only enables reuse for bodies under the cap. For anything over it, io.Copy stops short of resp.Body's EOF, the transport closes the connection instead of pooling it, and you've paid to read maxProbeBytes for nothing — partial drain is functionally identical to no drain.

The cap was cargo-culted from the read path on line 228, but the rationale doesn't carry over: that cap bounds Decode's memory allocation, whereas draining to io.Discard is O(1) memory. The only cost of an uncapped drain is time, and http.Client.Timeout (15s) already bounds that — its timer keeps running and interrupts Body reads even after Do returns.

Committed as 93da17c99 and pushed to PR #1320 (lint + build clean).

93da17cmirror: drain probe body to EOF, not capped, so reuse actually works A LimitReader cap on the drain defeats its own purpose: if the body exceeds the cap, io.Copy stops before resp.Body reaches EOF, and Go only recycles a connection whose body was read to EOF — so a capped drain is identical to no drain in exactly the case it's meant to handle. The maxProbeBytes cap on the read path bounds Decode's allocation; draining to io.Discard is O(1) memory, so no cap is needed there. The client Timeout still bounds how long the drain can run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 13446af914fa+7/-5

can we have an actual test for this?

3mo ago·21m

Done. There's now a real regression test, pushed to PR #1320.

What the test does

TestMirrorAdvertisesHead_ReusesConnection (repo_mirror_test.go) verifies the actual behavior the change exists for — connection reuse across probes:

  1. Test seam: I injected *http.Client into mirrorAdvertisesHead (the single caller passes probeClient). This lets the test use its own isolated client/idle-pool rather than the package global — otherwise the assertion would be flaky under t.Parallel(), since other parallel tests could evict our idle connection from the shared pool between calls.

  2. The setup: An httptest server returns a non-200 with a 64 KB body. mirrorAdvertisesHead returns at the StatusCode != 200 check without reading the body, so the deferred drain is the only thing that consumes it — isolating exactly the behavior under test.

  3. The assertion: A ConnState hook counts StateNew transitions across 5 probes. With the drain, all 5 share one connection. Without it, the body is left partially read and the transport opens a fresh connection each time.

Verification

I confirmed the test actually catches the regression — temporarily reverting the drain to a plain resp.Body.Close() made it fail with 5 new connections instead of 1, with the diagnostic message firing. Restored, it passes. The 64 KB body also guards against someone reintroducing a too-small LimitReader cap (a short cap would stop before EOF and fail the test too).

mise run fmt + mise run lint clean, full cli package tests green.

39754a1mirror: regression-test that the probe drains its body for conn reuse Inject the *http.Client into mirrorAdvertisesHead (caller passes probeClient) so the test can use an isolated, parallel-safe idle pool instead of the package global. The test serves a non-200 with a non-empty body — the function returns at the status check without reading the body, so the deferred drain is the only thing that consumes it. Counting StateNew transitions across five probes asserts they share one connection; removing or under-capping the drain leaves the body partially read and the count jumps to five. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ab3b94a4fb5a+68/-3