amadan

carlos / platform Public

host: fix tunnel reconnect hang and zero-delay redial storm (review fixes)

Two Important review findings against Task 4's tunnelLoop/dialTunnel:

1. dialTunnel had no deadline anywhere: an edge that accepts TCP and then
   never answers parked the loop forever with no retry, and — because
   tunnelStateVal was only written after dialTunnel returned — the
   heartbeat probe read a hung, armed tunnel as "" (disabled) the whole
   time. Fixed with a 10s net.Dialer/tls.DialWithDialer timeout on
   connect+TLS, a matching conn.SetDeadline around the /register
   write+ReadResponse (cleared before returning, so the live h2 session
   never inherits it), and moving the "reconnecting" state write to the
   top of every loop iteration instead of only the error branch.

2. A connection that died instantly after a successful dial redialed with
   zero delay. Because the edge's re-register semantics close the
   previous channel, two peers sharing CARLOS_TUNNEL_NAME (cloned env,
   agent started twice, a promote/adopt/restart overlap window) would
   mutually evict each other in an unbounded busy loop. Backoff now only
   resets to floor when the prior connection stayed up at least
   tunnelStableConnected (5s); below that it escalates like a dial
   failure, and the loop always sleeps before redialing either way.

Also: the backoff log line now reports the actual jittered sleep instead
of the pre-jitter value (reviewer minor #3). Added
TestTunnelLoopStateTransitions (drives tunnelLoop itself, asserts
""->"connected"->"reconnecting") and
TestTunnelLoopInertUnlessFullyConfigured (t.Setenv table over all seven
incomplete combinations of the three env vars) per reviewer minor #5.

go test ./internal/host/... -race and the full `go test ./... -count=1`
gate both clean (14/14 packages).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell e2576562e2fa9675c2a4814bd9d3ceefd4a88eb7 parent 7e13500
2 files changed, +143 −6
  • internal/host/tunnel_client.go +56 −6
  • internal/host/tunnel_client_test.go +87 −0
diff --git a/internal/host/tunnel_client.go b/internal/host/tunnel_client.go
index fced7fbbf..e5e38274b 100644
--- a/internal/host/tunnel_client.go
+++ b/internal/host/tunnel_client.go
@@ -36,6 +36,20 @@ func tunnelState() string {
const (
tunnelBackoffFloor = 250 * time.Millisecond
tunnelBackoffCap = 2 * time.Second
+ // tunnelStableConnected is how long a connection has to have stayed up
+ // before its loss resets backoff to the floor. Below this, the
+ // disconnect is treated like a dial failure and backoff keeps
+ // escalating: the edge's re-register-replaces-the-previous-channel
+ // semantics mean two peers sharing a CARLOS_TUNNEL_NAME (a cloned Pi
+ // env, the agent started twice, a promote/adopt/restart overlap window)
+ // would otherwise evict each other in an unbounded zero-sleep loop.
+ tunnelStableConnected = 5 * time.Second
+ // tunnelHandshakeTimeout bounds dial+TLS (via net.Dialer /
+ // tls.DialWithDialer) and, separately, the /register write+ReadResponse
+ // round trip: an edge that accepts the TCP conn and then never answers
+ // must not park tunnelLoop forever with no retry and no visible state
+ // change.
+ tunnelHandshakeTimeout = 10 * time.Second
)
// tunnelLoop dials CARLOS_TUNNEL_URL and serves handler over the inverted
@@ -54,22 +68,41 @@ func tunnelLoop(handler http.Handler) {
log.Printf("agent: tunnel loop armed (%s as %q)", rawURL, name)
backoff := tunnelBackoffFloor
for {
+ // Set BEFORE dialing, not only on error: dialTunnel can block for up
+ // to tunnelHandshakeTimeout on a black-holed handshake, and a stale
+ // "" or "connected" during that window would read to Task 6's
+ // heartbeat probe as either "never started" or "healthy" — both
+ // wrong while we're actually hung waiting on a dead edge.
+ tunnelStateVal.Store("reconnecting")
conn, err := dialTunnel(rawURL, name, token)
if err != nil {
- tunnelStateVal.Store("reconnecting")
- log.Printf("agent: tunnel dial: %v (retry in %s)", err, backoff)
- time.Sleep(backoff + rand.N(backoff/2))
+ sleep := backoff + rand.N(backoff/2)
+ log.Printf("agent: tunnel dial: %v (retry in %s)", err, sleep)
+ time.Sleep(sleep)
if backoff *= 2; backoff > tunnelBackoffCap {
backoff = tunnelBackoffCap
}
continue
}
- backoff = tunnelBackoffFloor
tunnelStateVal.Store("connected")
log.Printf("agent: tunnel connected as %q", name)
+ connectedAt := time.Now()
serveTunnel(conn, handler) // blocks until the conn dies
tunnelStateVal.Store("reconnecting")
log.Printf("agent: tunnel lost — re-dialing")
+ // A connection that stayed up a while earns a fast retry (the
+ // dual-WAN-failover / Wi-Fi-blip case); one that dies instantly does
+ // not — see tunnelStableConnected. Either way we always sleep some
+ // amount before redialing: a zero-delay retry is exactly the storm
+ // this guards against.
+ if time.Since(connectedAt) >= tunnelStableConnected {
+ backoff = tunnelBackoffFloor
+ } else if backoff *= 2; backoff > tunnelBackoffCap {
+ backoff = tunnelBackoffCap
+ }
+ sleep := backoff + rand.N(backoff/2)
+ log.Printf("agent: tunnel retry in %s", sleep)
+ time.Sleep(sleep)
}
}
@@ -91,24 +124,37 @@ func dialTunnel(rawURL, name, token string) (net.Conn, error) {
return nil, fmt.Errorf("tunnel url: %w", err)
}
addr := u.Host
+ dialer := &net.Dialer{Timeout: tunnelHandshakeTimeout}
var conn net.Conn
switch u.Scheme {
case "https":
if u.Port() == "" {
addr = net.JoinHostPort(u.Hostname(), "443")
}
- conn, err = tls.Dial("tcp", addr, &tls.Config{
+ // DialWithDialer applies dialer.Timeout to the TCP connect and the
+ // TLS handshake as a whole (crypto/tls docs) — a black-holed TLS
+ // peer fails the dial instead of hanging it.
+ conn, err = tls.DialWithDialer(dialer, "tcp", addr, &tls.Config{
ServerName: u.Hostname(),
NextProtos: []string{"http/1.1"}, // the handshake is 1.1; h2 starts after the 101
})
case "http":
- conn, err = net.Dial("tcp", addr)
+ conn, err = dialer.Dial("tcp", addr)
default:
return nil, fmt.Errorf("tunnel url: unsupported scheme %q", u.Scheme)
}
if err != nil {
return nil, err
}
+ // The dial above only bounds TCP connect + TLS handshake; the
+ // /register write+read is a separate hang risk (an edge that accepts
+ // the conn and then never answers). Bound it too, and clear the
+ // deadline before returning success — the live h2 session that follows
+ // must not inherit a stale deadline from the handshake.
+ if err := conn.SetDeadline(time.Now().Add(tunnelHandshakeTimeout)); err != nil {
+ conn.Close()
+ return nil, fmt.Errorf("tunnel: set handshake deadline: %w", err)
+ }
req := &http.Request{
Method: "GET",
URL: &url.URL{Path: "/register"},
@@ -135,6 +181,10 @@ func dialTunnel(rawURL, name, token string) (net.Conn, error) {
conn.Close()
return nil, fmt.Errorf("tunnel register: %s", resp.Status)
}
+ if err := conn.SetDeadline(time.Time{}); err != nil {
+ conn.Close()
+ return nil, fmt.Errorf("tunnel: clear handshake deadline: %w", err)
+ }
if br.Buffered() > 0 {
return bufferedConn{r: io.MultiReader(br, conn), Conn: conn}, nil
}
diff --git a/internal/host/tunnel_client_test.go b/internal/host/tunnel_client_test.go
index 035173b59..b9315999a 100644
--- a/internal/host/tunnel_client_test.go
+++ b/internal/host/tunnel_client_test.go
@@ -6,6 +6,7 @@ import (
"net/http"
"net/http/httptest"
"testing"
+ "time"
"golang.org/x/net/http2"
)
@@ -72,3 +73,89 @@ func TestDialTunnelRejectsNon101(t *testing.T) {
t.Fatal("want error on 401 handshake")
}
}
+
+// TestTunnelLoopStateTransitions drives tunnelLoop (not dialTunnel/
+// serveTunnel directly) against a fake edge that hijacks, answers 101, then
+// holds the raw conn open until the test releases it — enough to observe
+// tunnelState() go "" -> "connected", and then "connected" -> "reconnecting"
+// once the conn is torn down. This is the state Task 6's heartbeat probe
+// reads, so its transitions are worth asserting directly, not just inferring
+// from dialTunnel/serveTunnel behavior.
+func TestTunnelLoopStateTransitions(t *testing.T) {
+ release := make(chan struct{})
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ conn, bufrw, err := w.(http.Hijacker).Hijack()
+ if err != nil {
+ t.Errorf("hijack: %v", err)
+ return
+ }
+ fmt.Fprintf(bufrw, "HTTP/1.1 101 Switching Protocols\r\nUpgrade: carlos-tunnel/1\r\nConnection: Upgrade\r\n\r\n")
+ bufrw.Flush()
+ <-release
+ conn.Close()
+ }))
+ defer srv.Close()
+
+ t.Setenv("CARLOS_TUNNEL_URL", srv.URL)
+ t.Setenv("CARLOS_TUNNEL_TOKEN", "sekrit")
+ t.Setenv("CARLOS_TUNNEL_NAME", "pi-box")
+
+ // tunnelLoop never returns while configured (by design — it retries
+ // forever), so this goroutine outlives the test; the fake edge is torn
+ // down via defer, and any further redial attempts just fail fast against
+ // a closed listener.
+ go tunnelLoop(http.NewServeMux())
+
+ waitForTunnelState(t, "connected")
+
+ close(release)
+
+ waitForTunnelState(t, "reconnecting")
+}
+
+// TestTunnelLoopInertUnlessFullyConfigured checks the documented convention:
+// tunnelLoop must return immediately — never dial, never block — unless all
+// three of CARLOS_TUNNEL_URL/_TOKEN/_NAME are set.
+func TestTunnelLoopInertUnlessFullyConfigured(t *testing.T) {
+ cases := []struct {
+ name, url, token, tname string
+ }{
+ {"all unset", "", "", ""},
+ {"url only", "http://tunnel.invalid", "", ""},
+ {"token only", "", "tok", ""},
+ {"name only", "", "", "pi-box"},
+ {"url and token", "http://tunnel.invalid", "tok", ""},
+ {"url and name", "http://tunnel.invalid", "", "pi-box"},
+ {"token and name", "", "tok", "pi-box"},
+ }
+ for _, c := range cases {
+ t.Run(c.name, func(t *testing.T) {
+ t.Setenv("CARLOS_TUNNEL_URL", c.url)
+ t.Setenv("CARLOS_TUNNEL_TOKEN", c.token)
+ t.Setenv("CARLOS_TUNNEL_NAME", c.tname)
+
+ done := make(chan struct{})
+ go func() {
+ tunnelLoop(http.NewServeMux())
+ close(done)
+ }()
+ select {
+ case <-done:
+ case <-time.After(500 * time.Millisecond):
+ t.Fatal("tunnelLoop did not return promptly with an incomplete env config")
+ }
+ })
+ }
+}
+
+func waitForTunnelState(t *testing.T, want string) {
+ t.Helper()
+ deadline := time.Now().Add(2 * time.Second)
+ for time.Now().Before(deadline) {
+ if tunnelState() == want {
+ return
+ }
+ time.Sleep(5 * time.Millisecond)
+ }
+ t.Fatalf("tunnelState() = %q, want %q before deadline", tunnelState(), want)
+}