refactor: address PR review feedback for healthcheck and proxy

Critical fixes:
- Add detailed comment explaining hasConnectedOnce guard prevents goroutine leaks on reconnects
- Verify port detection logic correctly handles URLs with explicit ports

Documentation improvements:
- Document 3-second timeout assumptions in listen.go for local development context
- Document timeout parameter guidance in healthcheck.CheckServerHealth function
- Fix re-export comment from 'backward compatibility' to 'convenience'
Test coverage:
- Add TestCheckServerHealth_PortInURL to verify explicit ports are not overwritten
- Confirms fix for edge case where localhost:8080 would not become localhost:8080:80
All tests passing: go test ./pkg/listen/...

Author: leggetter

pkg/listen/healthcheck.go

@@ -7,7 +7,7 @@ import (
 	"github.com/hookdeck/hookdeck-cli/pkg/listen/healthcheck"
 )
 
-// Re-export types and constants from healthcheck subpackage for backward compatibility
+// Re-export types and constants from healthcheck subpackage for convenience
 type ServerHealthStatus = healthcheck.ServerHealthStatus
 type HealthCheckResult = healthcheck.HealthCheckResult
 

pkg/listen/healthcheck/healthcheck.go

@@ -27,7 +27,10 @@ type HealthCheckResult struct {
 	Duration  time.Duration
 }
 
-// CheckServerHealth performs a TCP connection check to the target URL
+// CheckServerHealth performs a TCP connection check to verify a server is listening.
+// The timeout parameter should be appropriate for the deployment context:
+// - Local development: 3s is typically sufficient
+// - Production/edge: May require longer timeouts due to network conditions
 func CheckServerHealth(targetURL *url.URL, timeout time.Duration) HealthCheckResult {
 	start := time.Now()
 

pkg/listen/healthcheck/healthcheck_test.go

@@ -1,6 +1,7 @@
 package healthcheck
 
 import (
+	"fmt"
 	"net"
 	"net/http"
 	"net/http/httptest"
@@ -170,3 +171,29 @@ func TestFormatHealthMessage_NilError(t *testing.T) {
 		t.Errorf("Expected message to contain 'unknown error' when error is nil")
 	}
 }
+
+func TestCheckServerHealth_PortInURL(t *testing.T) {
+	// Create a server on a non-standard port
+	listener, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Fatalf("Failed to create listener: %v", err)
+	}
+	defer listener.Close()
+
+	// Get the actual port assigned by the OS
+	addr := listener.Addr().(*net.TCPAddr)
+	targetURL, _ := url.Parse(fmt.Sprintf("http://localhost:%d/path", addr.Port))
+
+	// Perform health check
+	result := CheckServerHealth(targetURL, 3*time.Second)
+
+	// Verify that the health check succeeded
+	// This confirms that when a port is already in the URL, we don't append
+	// a default port (which would cause localhost:8080 to become localhost:8080:80)
+	if !result.Healthy {
+		t.Errorf("Expected healthy=true for server with port in URL, got false: %v", result.Error)
+	}
+	if result.Error != nil {
+		t.Errorf("Expected no error for server with port in URL, got: %v", result.Error)
+	}
+}

pkg/listen/listen.go

@@ -124,6 +124,9 @@ Specify a single destination to update the path. For example, pass a connection
 	}
 
 	// Perform initial health check on target server
+	// Using 3-second timeout optimized for local development scenarios.
+	// This assumes low latency to localhost. For production/edge deployments,
+	// this timeout may need to be configurable in future iterations.
 	healthCheckTimeout := 3 * time.Second
 	healthResult := CheckServerHealth(URL, healthCheckTimeout)
 

pkg/listen/proxy/proxy.go

@@ -158,8 +158,10 @@ func (p *Proxy) Run(parentCtx context.Context) error {
 			<-p.webSocketClient.Connected()
 			p.renderer.OnConnected()
 
-			// Only start health monitoring on first successful connection
-			// to prevent goroutine leaks on reconnects
+			// Only start health monitoring on first successful connection to prevent
+			// goroutine leaks on reconnects. The hasConnectedOnce guard ensures that
+			// even if the websocket reconnects multiple times (which happens in the
+			// Run() loop), we only spawn the health monitor goroutine once.
 			if !hasConnectedOnce {
 				hasConnectedOnce = true