Skip to content

mirror: drain probe body before close so the idle pool works#1320

Merged
Soph merged 3 commits into
mainfrom
probe-drain-conn-reuse
Jun 2, 2026
Merged

mirror: drain probe body before close so the idle pool works#1320
Soph merged 3 commits into
mainfrom
probe-drain-conn-reuse

Conversation

@Soph
Copy link
Copy Markdown
Collaborator

@Soph Soph commented Jun 2, 2026

https://entire.io/gh/entireio/cli/trails/474

Summary

probeClient (used by the mirror clone-readiness loop in repo_mirror_probe.go) is deliberately built with a small idle pool (MaxIdleConns: 2) and a 90s IdleConnTimeout, specifically to reuse the TLS session across the 2s 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 it, and the sr.Decode/adv.Decode error returns leave the body partially read. So the connection was closed instead of pooled — every 2s probe paid a fresh TLS handshake, defeating the pool's stated purpose. Over a 30-minute wait (~900 probes) that's 900 handshakes instead of one reused connection.

Change

Drain the body (bounded by maxProbeBytes to match the existing read cap) before closing, so the transport can recycle the connection.

Testing

  • mise run fmt + mise run lint — clean
  • go build ./cmd/entire/... — clean

🤖 Generated with Claude Code


Note

Low Risk
Localized HTTP client lifecycle fix in the mirror readiness probe; no auth, data, or API contract changes.

Overview
Fixes mirror clone-readiness probes so probeClient can actually reuse idle connections and TLS sessions between ~2s ticks.

In mirrorAdvertisesHead, the response body is now drained (capped at maxProbeBytes, same as the decode path) before Close(), because partial reads from decode/error paths prevented Go’s transport from returning connections to the pool—long waits could repeat a full TLS handshake on every probe instead of one reused connection.

Reviewed by Cursor Bugbot for commit 4aed46f. Configure here.

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
@Soph Soph requested a review from a team as a code owner June 2, 2026 07:29
Copilot AI review requested due to automatic review settings June 2, 2026 07:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes connection reuse in the mirror clone-readiness probe loop. probeClient was configured with an idle pool and 90s IdleConnTimeout to reuse TLS sessions across ~2s probes, but mirrorAdvertisesHead closed the response body without draining it. Because Go's transport only returns connections to the idle pool when bodies are read to EOF before Close(), every probe was paying a fresh TLS handshake — defeating the pool entirely during long waits.

Changes:

  • In mirrorAdvertisesHead, drain the response body (capped to maxProbeBytes to mirror the existing read cap) before Close() so the transport can recycle the connection.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 4aed46f. Configure here.

Comment thread cmd/entire/cli/repo_mirror_probe.go Outdated
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
pjbgf
pjbgf previously approved these changes Jun 2, 2026
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
@Soph Soph merged commit d4dd696 into main Jun 2, 2026
9 checks passed
@Soph Soph deleted the probe-drain-conn-reuse branch June 2, 2026 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants