Improve reliability, linting, add deploy target

Author: tstromberg

Makefile

@@ -1,4 +1,4 @@
-.PHONY: all build test lint clean fmt vet
+.PHONY: all build test lint clean fmt vet deploy
 
 all: fmt vet lint test build
 
@@ -24,6 +24,9 @@ run-server:
 
 run-client:
 	go run ./cmd/client
+
+deploy:
+	./hacks/deploy.sh cmd/server/
 # BEGIN: lint-install .
 # http://github.com/codeGROOVE-dev/lint-install
 

cmd/server/main.go

@@ -48,18 +48,19 @@ func main() {
 	flag.Parse()
 
 	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
 
 	// Get webhook secret from flag or environment variable
 	webhookSecretValue := *webhookSecret
 
 	// Validate webhook secret is configured (REQUIRED for security)
 	if webhookSecretValue == "" {
+		cancel()
 		log.Fatal("ERROR: Webhook secret is required for security. Set -webhook-secret or GITHUB_WEBHOOK_SECRET environment variable.")
 	}
 
 	// Validate allowed events is configured (REQUIRED)
 	if *allowedEvents == "" {
+		cancel()
 		log.Fatal("ERROR: Allowed events must be specified. Set -allowed-events or " +
 			"ALLOWED_WEBHOOK_EVENTS environment variable. Use '*' to allow all events.")
 	}
@@ -77,6 +78,9 @@ func main() {
 		log.Printf("Allowing webhook event types: %v", allowedEventTypes)
 	}
 
+	// Defer cancel after all fatal validations
+	defer cancel()
+
 	// CORS support removed - WebSocket clients should handle auth via Authorization header
 
 	h := hub.NewHub()

hacks/deploy.sh

@@ -1,4 +1,5 @@
-#!/usr/bin/env bash -eux -o pipefail
+#!/usr/bin/env bash
+set -eux -o pipefail
 # Deploys Go program to Cloud Run - from cwd or arg[1].
 cd "${1:-.}"
 
@@ -10,7 +11,7 @@ SA="$APP@$PROJECT.iam.gserviceaccount.com"
 gcloud iam service-accounts describe "$SA" &>/dev/null ||
 	gcloud iam service-accounts create "$APP" --project="$PROJECT"
 
-grep -q gcr.io $HOME/.docker/config.json ||
+grep -q gcr.io "$HOME"/.docker/config.json ||
     gcloud auth configure-docker gcr.io
 
 KO_DOCKER_REPO="gcr.io/$PROJECT/$APP" ko publish . |

pkg/client/example_test.go

@@ -3,7 +3,6 @@ package client_test
 import (
 	"context"
 	"fmt"
-	"io"
 	"log"
 	"log/slog"
 	"os"
@@ -81,7 +80,7 @@ func ExampleClient_gracefulShutdown() {
 
 func ExampleClient_customLogger() {
 	// Example 1: Silence all logs
-	silentLogger := slog.New(slog.NewTextHandler(io.Discard, nil))
+	silentLogger := slog.New(slog.DiscardHandler)
 
 	// Example 2: JSON logging to a file
 	logFile, err := os.Create("client.log")

pkg/hub/websocket.go

@@ -22,7 +22,7 @@ import (
 // Constants for WebSocket timeouts and limits.
 const (
 	pingInterval        = 54 * time.Second
-	readDeadline        = 60 * time.Second
+	readTimeout         = 60 * time.Second // Must be > pingInterval to avoid false timeouts
 	writeTimeout        = 10 * time.Second
 	minTokenLength      = 40   // Minimum GitHub token length
 	maxTokenLength      = 255  // Maximum GitHub token length
@@ -376,22 +376,6 @@ func (h *WebSocketHandler) validateAuth(ctx context.Context, ws *websocket.Conn,
 	return userOrgs, nil
 }
 
-// handleClientPing responds to a ping message from the client.
-func (*WebSocketHandler) handleClientPing(ws *websocket.Conn, msgMap map[string]any, clientID string) {
-	pong := map[string]any{"type": "pong"}
-	// Echo back any sequence number if present
-	if seq, ok := msgMap["seq"]; ok {
-		pong["seq"] = seq
-	}
-	if err := ws.SetWriteDeadline(time.Now().Add(writeTimeout)); err != nil {
-		log.Printf("failed to set write deadline for pong to client %s: %v", clientID, err)
-		return
-	}
-	if err := websocket.JSON.Send(ws, pong); err != nil {
-		log.Printf("failed to send pong to client %s: %v", clientID, err)
-	}
-}
-
 // Handle handles a WebSocket connection.
 //
 //nolint:funlen,gocyclo // This function orchestrates the complete WebSocket lifecycle and cannot be split without losing clarity
@@ -495,7 +479,7 @@ func (h *WebSocketHandler) Handle(ws *websocket.Conn) {
 	}
 	defer h.connLimiter.Remove(ip)
 
-	// Set read deadline for initial subscription
+	// Set read deadline for initial subscription (shorter timeout for handshake)
 	if err := ws.SetDeadline(time.Now().Add(5 * time.Second)); err != nil {
 		log.Printf("failed to set deadline for %s: %v", ip, err)
 		return
@@ -676,18 +660,15 @@ func (h *WebSocketHandler) Handle(ws *websocket.Conn) {
 	go client.Run(ctx, pingInterval, writeTimeout)
 
 	// Handle incoming messages with responsive shutdown
-	// Use a shorter read timeout to make shutdown more responsive
-	readTimeout := 2 * time.Second
-
 	// Create a ticker for periodic context checks during blocking reads
 	contextCheckTicker := time.NewTicker(1 * time.Second)
 	defer contextCheckTicker.Stop()
 
+	// Set initial read deadline - must be longer than pingInterval to avoid false timeouts
 	if err := ws.SetReadDeadline(time.Now().Add(readTimeout)); err != nil {
 		log.Printf("failed to set read deadline for %s: %v", ip, err)
 		return
 	}
-	// Read deadline set for responsive shutdown
 
 	// Message read loop with responsive shutdown
 	for {
@@ -717,7 +698,7 @@ func (h *WebSocketHandler) Handle(ws *websocket.Conn) {
 				log.Printf("client %s connection already closed", client.ID)
 			case strings.Contains(err.Error(), "i/o timeout"):
 				log.Printf("TIMEOUT: client %s read timeout at %s (no messages received for %v)",
-					client.ID, time.Now().Format(time.RFC3339), readDeadline)
+					client.ID, time.Now().Format(time.RFC3339), readTimeout)
 			default:
 				log.Printf("client %s read error: %v", client.ID, err)
 			}
@@ -739,7 +720,17 @@ func (h *WebSocketHandler) Handle(ws *websocket.Conn) {
 					continue
 				case "ping":
 					// Client sent us a ping, send pong back
-					h.handleClientPing(ws, msgMap, client.ID)
+					pong := map[string]any{"type": "pong"}
+					if seq, ok := msgMap["seq"]; ok {
+						pong["seq"] = seq
+					}
+					if err := ws.SetWriteDeadline(time.Now().Add(writeTimeout)); err != nil {
+						log.Printf("failed to set write deadline for pong to client %s: %v", client.ID, err)
+						continue
+					}
+					if err := websocket.JSON.Send(ws, pong); err != nil {
+						log.Printf("failed to send pong to client %s: %v", client.ID, err)
+					}
 					continue
 				case "keepalive", "heartbeat":
 					// Common keepalive messages - just acknowledge receipt

pkg/webhook/handler.go

@@ -138,12 +138,20 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	prURL := ExtractPRURL(eventType, payload)
 	if prURL == "" {
 		// Log full payload to understand the structure
-		payloadJSON, _ := json.MarshalIndent(payload, "", "  ")
-		logger.Info("no PR URL found in event - full payload", logger.Fields{
-			"event_type":  eventType,
-			"delivery_id": deliveryID,
-			"payload":     string(payloadJSON),
-		})
+		payloadJSON, err := json.MarshalIndent(payload, "", "  ")
+		if err != nil {
+			logger.Warn("failed to marshal payload for logging", logger.Fields{
+				"event_type":  eventType,
+				"delivery_id": deliveryID,
+				"error":       err.Error(),
+			})
+		} else {
+			logger.Info("no PR URL found in event - full payload", logger.Fields{
+				"event_type":  eventType,
+				"delivery_id": deliveryID,
+				"payload":     string(payloadJSON),
+			})
+		}
 		w.WriteHeader(http.StatusOK)
 		return
 	}