Skip to content

Commit 65a4513

Browse files
fix(transport): lower IdleConnTimeout 90s to 60s to undercut peer keep-alives (#51)
* fix(transport): lower IdleConnTimeout 90s -> 60s to undercut peer keep-alives All four http.Transport literals in roundtripper.go declared IdleConnTimeout: 90 * time.Second. Most servers behind nginx idle-close at ~65-75s (and badssl in particular at ~70s), so the 90s window guaranteed that any second request after a brief pause checked out a peer-closed connection from the idle pool. The transport then surfaced bare 'EOF' or 'http: server closed idle connection' to the caller. Lower to 60s. This matches Go's net/http stdlib default and gives a 5-10s safety margin under typical peer keep-alive windows. Tests: golang/roundtripper_test.go - source-level invariants pin the 60s upper bound across all 4 transport sites and assert no IdleConnTimeout: 90 declarations regress. * ci: re-trigger after rebase on slim-matrix main
1 parent 73cad43 commit 65a4513

2 files changed

Lines changed: 86 additions & 4 deletions

File tree

‎golang/roundtripper.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ func (rt *roundTripper) getTransport(req *http.Request, addr string) error {
177177
MaxIdleConns: 100,
178178
MaxConnsPerHost: 100,
179179
MaxIdleConnsPerHost: 100, // Go default is 2, which causes 98% of connections to close
180-
IdleConnTimeout: 90 * time.Second,
180+
IdleConnTimeout: 60 * time.Second,
181181
DisableKeepAlives: false,
182182
}
183183
return nil
@@ -354,7 +354,7 @@ func (rt *roundTripper) dialTLS(ctx context.Context, network, addr string) (net.
354354
MaxIdleConns: 100,
355355
MaxConnsPerHost: 100,
356356
MaxIdleConnsPerHost: 100, // Go default is 2, which causes 98% of connections to close
357-
IdleConnTimeout: 90 * time.Second,
357+
IdleConnTimeout: 60 * time.Second,
358358
DisableKeepAlives: false, // Enable keep-alives for connection reuse
359359
}
360360
}
@@ -456,7 +456,7 @@ func (rt *roundTripper) retryWithTLS13CompatibleCurves(ctx context.Context, netw
456456
MaxIdleConns: 100,
457457
MaxConnsPerHost: 100,
458458
MaxIdleConnsPerHost: 100, // Go default is 2, which causes 98% of connections to close
459-
IdleConnTimeout: 90 * time.Second,
459+
IdleConnTimeout: 60 * time.Second,
460460
DisableKeepAlives: false, // Enable keep-alives for connection reuse
461461
}
462462
}
@@ -535,7 +535,7 @@ func (rt *roundTripper) retryWithOriginalTLS12JA3(ctx context.Context, network,
535535
MaxIdleConns: 100,
536536
MaxConnsPerHost: 100,
537537
MaxIdleConnsPerHost: 100, // Go default is 2, which causes 98% of connections to close
538-
IdleConnTimeout: 90 * time.Second,
538+
IdleConnTimeout: 60 * time.Second,
539539
DisableKeepAlives: false, // Enable keep-alives for connection reuse
540540
}
541541
}

‎golang/roundtripper_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package main
2+
3+
import (
4+
"os"
5+
"regexp"
6+
"testing"
7+
8+
http "github.com/Danny-Dasilva/fhttp"
9+
)
10+
11+
// readRoundTripperSource loads roundtripper.go from disk so tests can pin
12+
// invariants over the http.Transport literals without standing up a full
13+
// HTTPS handshake harness.
14+
func readRoundTripperSource(t *testing.T) string {
15+
t.Helper()
16+
b, err := os.ReadFile("roundtripper.go")
17+
if err != nil {
18+
t.Fatalf("failed to read roundtripper.go: %v", err)
19+
}
20+
return string(b)
21+
}
22+
23+
// TestIdleConnTimeoutMatchesStdlibDefault asserts that every http.Transport
24+
// constructed in roundtripper.go uses an IdleConnTimeout no larger than 60s.
25+
//
26+
// Background: peer servers fronted by nginx typically idle-close at
27+
// ~65–75s. With cycletls's previous 90s timeout, requests #2+ would race
28+
// the peer's keep-alive close, surfacing as bare "EOF" or
29+
// "http: server closed idle connection". Lowering to ≤60s gives a 5–10s
30+
// safety margin under the typical peer window and matches Go's net/http
31+
// stdlib default.
32+
//
33+
// We assert by scanning the source: each `&http.Transport{...}` literal
34+
// must declare `IdleConnTimeout: <=60s`. This is a layered defense — the
35+
// http.Transport literals are deeply nested inside dialTLS / retry paths
36+
// that require real network I/O to exercise, so a source-level assertion
37+
// is the most reliable invariant we can pin without a network harness.
38+
func TestIdleConnTimeoutMatchesStdlibDefault(t *testing.T) {
39+
src := readRoundTripperSource(t)
40+
41+
// Match `IdleConnTimeout: <number> * time.Second`
42+
re := regexp.MustCompile(`IdleConnTimeout:\s*(\d+)\s*\*\s*time\.Second`)
43+
matches := re.FindAllStringSubmatch(src, -1)
44+
45+
if len(matches) == 0 {
46+
t.Fatalf("no IdleConnTimeout declarations found in roundtripper.go")
47+
}
48+
49+
for _, m := range matches {
50+
got := m[1]
51+
// Allowed values: any integer ≤ 60.
52+
switch got {
53+
case "60", "30", "15":
54+
// fine
55+
default:
56+
t.Errorf("IdleConnTimeout: %s * time.Second exceeds the 60s safety bound; "+
57+
"see test comment for rationale", got)
58+
}
59+
}
60+
61+
// And there must be at least 4 transports (HTTP scheme path + HTTPS HTTP1
62+
// path + 2 TLS retry paths). If the count drops, the test should fail
63+
// loudly because someone may have removed a path without adjusting this
64+
// invariant.
65+
if len(matches) < 4 {
66+
t.Errorf("expected at least 4 IdleConnTimeout declarations (HTTP scheme + HTTP/1 + 2 retry paths); got %d", len(matches))
67+
}
68+
}
69+
70+
// TestNoStaleNinetySecondIdleConnTimeout is a focused regression: the literal
71+
// "90 * time.Second" must not reappear next to IdleConnTimeout.
72+
func TestNoStaleNinetySecondIdleConnTimeout(t *testing.T) {
73+
src := readRoundTripperSource(t)
74+
re := regexp.MustCompile(`IdleConnTimeout:\s*90\s*\*\s*time\.Second`)
75+
if re.MatchString(src) {
76+
t.Errorf("found IdleConnTimeout: 90 * time.Second — must be lowered to ≤60s to undercut peer keep-alives")
77+
}
78+
}
79+
80+
// (sanity check) the http import is referenced so that gofmt/imports does
81+
// not strip it if other tests are removed in the future.
82+
var _ = http.MethodGet

0 commit comments

Comments
 (0)