Silence context.Canceled errors from client disconnects

Add web.ServerError and web.IsClientDisconnect helpers to centralize
the check, replacing repetitive inline handling across all API and UI
handlers. Also adds missing error logging in the popularity handler.

Author: pierres

internal/packages/handler.go

@@ -1,7 +1,6 @@
 package packages
 
 import (
-	"log/slog"
 	"net/http"
 
 	"pkgstatsd/internal/web"
@@ -30,8 +29,7 @@ func (h *Handler) HandleGet(w http.ResponseWriter, r *http.Request) {
 
 	pkg, err := h.repo.FindByName(r.Context(), name, startMonth, endMonth)
 	if err != nil {
-		slog.Error("failed to find package", "error", err)
-		web.InternalServerError(w, "internal server error")
+		web.ServerError(w, "failed to find package", err)
 		return
 	}
 
@@ -59,8 +57,7 @@ func (h *Handler) HandleList(w http.ResponseWriter, r *http.Request) {
 
 	list, err := h.repo.FindAll(r.Context(), query, startMonth, endMonth, limit, offset)
 	if err != nil {
-		slog.Error("failed to list packages", "error", err)
-		web.InternalServerError(w, "internal server error")
+		web.ServerError(w, "failed to list packages", err)
 		return
 	}
 
@@ -88,8 +85,7 @@ func (h *Handler) HandleSeries(w http.ResponseWriter, r *http.Request) {
 
 	list, err := h.repo.FindSeriesByName(r.Context(), name, startMonth, endMonth, limit, offset)
 	if err != nil {
-		slog.Error("failed to find package series", "error", err)
-		web.InternalServerError(w, "internal server error")
+		web.ServerError(w, "failed to find package series", err)
 		return
 	}
 

internal/packages/handler_test.go

@@ -615,6 +615,66 @@ func TestHandleList_PaginationValidCases(t *testing.T) {
 	}
 }
 
+func TestHandleGet_ContextCanceled(t *testing.T) {
+	repo := &mockRepository{
+		findByNameFunc: func(_ context.Context, _ string, _, _ int) (*PackagePopularity, error) {
+			return nil, context.Canceled
+		},
+	}
+
+	mux := newTestMux(repo)
+	req := httptest.NewRequest(http.MethodGet, "/api/packages/pacman", nil)
+	rr := httptest.NewRecorder()
+	mux.ServeHTTP(rr, req)
+
+	if rr.Code != http.StatusOK {
+		t.Errorf("expected no response written (status 200), got %d", rr.Code)
+	}
+	if rr.Body.Len() != 0 {
+		t.Errorf("expected empty body, got %q", rr.Body.String())
+	}
+}
+
+func TestHandleList_ContextCanceled(t *testing.T) {
+	repo := &mockRepository{
+		findAllFunc: func(_ context.Context, _ string, _, _, _, _ int) (*PackagePopularityList, error) {
+			return nil, fmt.Errorf("count packages: %w", context.Canceled)
+		},
+	}
+
+	mux := newTestMux(repo)
+	req := httptest.NewRequest(http.MethodGet, "/api/packages", nil)
+	rr := httptest.NewRecorder()
+	mux.ServeHTTP(rr, req)
+
+	if rr.Code != http.StatusOK {
+		t.Errorf("expected no response written (status 200), got %d", rr.Code)
+	}
+	if rr.Body.Len() != 0 {
+		t.Errorf("expected empty body, got %q", rr.Body.String())
+	}
+}
+
+func TestHandleSeries_ContextCanceled(t *testing.T) {
+	repo := &mockRepository{
+		findSeriesByNameFunc: func(_ context.Context, _ string, _, _, _, _ int) (*PackagePopularityList, error) {
+			return nil, context.Canceled
+		},
+	}
+
+	mux := newTestMux(repo)
+	req := httptest.NewRequest(http.MethodGet, "/api/packages/pacman/series", nil)
+	rr := httptest.NewRecorder()
+	mux.ServeHTTP(rr, req)
+
+	if rr.Code != http.StatusOK {
+		t.Errorf("expected no response written (status 200), got %d", rr.Code)
+	}
+	if rr.Body.Len() != 0 {
+		t.Errorf("expected empty body, got %q", rr.Body.String())
+	}
+}
+
 func TestHandleList_PaginationInvalidCases(t *testing.T) {
 	tests := []struct {
 		name string

internal/popularity/handler.go

@@ -44,7 +44,7 @@ func (h *Handler[T, L]) HandleGet(w http.ResponseWriter, r *http.Request) {
 
 	item, err := h.repo.FindByIdentifier(r.Context(), identifier, startMonth, endMonth)
 	if err != nil {
-		web.InternalServerError(w, "internal server error")
+		web.ServerError(w, "failed to find item", err)
 		return
 	}
 
@@ -72,7 +72,7 @@ func (h *Handler[T, L]) HandleList(w http.ResponseWriter, r *http.Request) {
 
 	list, err := h.repo.FindAll(r.Context(), query, startMonth, endMonth, limit, offset)
 	if err != nil {
-		web.InternalServerError(w, "internal server error")
+		web.ServerError(w, "failed to list items", err)
 		return
 	}
 
@@ -100,7 +100,7 @@ func (h *Handler[T, L]) HandleSeries(w http.ResponseWriter, r *http.Request) {
 
 	list, err := h.repo.FindSeries(r.Context(), identifier, startMonth, endMonth, limit, offset)
 	if err != nil {
-		web.InternalServerError(w, "internal server error")
+		web.ServerError(w, "failed to find item series", err)
 		return
 	}
 

internal/sitemap/handler.go

@@ -62,6 +62,9 @@ func (h *Handler) HandleSitemap(w http.ResponseWriter, r *http.Request) {
 
 	list, err := h.repo.FindAll(r.Context(), "", currentMonth, currentMonth, packageLimit, 0)
 	if err != nil {
+		if web.IsClientDisconnect(err) {
+			return
+		}
 		slog.Error("failed to fetch packages for sitemap", "error", err)
 	} else {
 		for _, pkg := range list.PackagePopularities {

internal/sitemap/handler_test.go

@@ -2,6 +2,7 @@ package sitemap
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -13,6 +14,10 @@ import (
 
 type mockRepo struct{}
 
+type errorRepo struct {
+	err error
+}
+
 func (m *mockRepo) FindByName(_ context.Context, _ string, _, _ int) (*packages.PackagePopularity, error) {
 	return nil, nil
 }
@@ -76,6 +81,57 @@ func TestHandleSitemap(t *testing.T) {
 	}
 }
 
+func (m *errorRepo) FindByName(_ context.Context, _ string, _, _ int) (*packages.PackagePopularity, error) {
+	return nil, m.err
+}
+
+func (m *errorRepo) FindSeriesByName(_ context.Context, _ string, _, _, _, _ int) (*packages.PackagePopularityList, error) {
+	return nil, m.err
+}
+
+func (m *errorRepo) FindAll(_ context.Context, _ string, _, _, _, _ int) (*packages.PackagePopularityList, error) {
+	return nil, m.err
+}
+
+func TestHandleSitemapReturnsEarlyOnClientDisconnect(t *testing.T) {
+	handler := NewHandler(&errorRepo{err: context.Canceled})
+
+	req := httptest.NewRequest(http.MethodGet, "/sitemap.xml", nil)
+	req.Host = "example.com"
+	rr := httptest.NewRecorder()
+
+	handler.HandleSitemap(rr, req)
+
+	if rr.Code != http.StatusOK {
+		t.Errorf("expected status 200, got %d", rr.Code)
+	}
+	if rr.Body.Len() != 0 {
+		t.Errorf("expected empty body on client disconnect, got %d bytes", rr.Body.Len())
+	}
+}
+
+func TestHandleSitemapServesPartialOnError(t *testing.T) {
+	handler := NewHandler(&errorRepo{err: errors.New("db error")})
+
+	req := httptest.NewRequest(http.MethodGet, "/sitemap.xml", nil)
+	req.Host = "example.com"
+	rr := httptest.NewRecorder()
+
+	handler.HandleSitemap(rr, req)
+
+	if rr.Code != http.StatusOK {
+		t.Errorf("expected status 200, got %d", rr.Code)
+	}
+
+	body := rr.Body.String()
+	if !strings.Contains(body, "<loc>http://example.com/</loc>") {
+		t.Error("expected static URLs in partial sitemap")
+	}
+	if strings.Contains(body, "<loc>http://example.com/packages/linux</loc>") {
+		t.Error("expected no package URLs on error")
+	}
+}
+
 func TestLastDayOfMonth(t *testing.T) {
 	tests := []struct {
 		yearMonth int

internal/submit/handler.go

@@ -2,7 +2,6 @@ package submit
 
 import (
 	"fmt"
-	"log/slog"
 	"net/http"
 	"net/netip"
 	"time"
@@ -39,8 +38,7 @@ func (h *Handler) HandleSubmit(w http.ResponseWriter, r *http.Request) {
 	anonymizedIP := AnonymizeIP(clientIP)
 	allowed, retryAfter, err := h.limiter.Allow(r.Context(), anonymizedIP)
 	if err != nil {
-		slog.Error("rate limit check failed", "error", err)
-		web.InternalServerError(w, "internal server error")
+		web.ServerError(w, "rate limit check failed", err)
 		return
 	}
 
@@ -72,8 +70,7 @@ func (h *Handler) HandleSubmit(w http.ResponseWriter, r *http.Request) {
 	mirrorURL := FilterMirrorURL(req.Pacman.Mirror)
 
 	if err := h.repo.SaveSubmission(r.Context(), req, mirrorURL); err != nil {
-		slog.Error("failed to save submission", "error", err)
-		web.InternalServerError(w, "failed to save submission")
+		web.ServerError(w, "failed to save submission", err)
 		return
 	}
 

internal/ui/layout/render.go

@@ -1,12 +1,12 @@
 package layout
 
 import (
-	"context"
-	"errors"
 	"log/slog"
 	"net/http"
 
 	"github.com/a-h/templ"
+
+	"pkgstatsd/internal/web"
 )
 
 func Render(w http.ResponseWriter, r *http.Request, page Page, content templ.Component) {
@@ -24,12 +24,14 @@ func Render(w http.ResponseWriter, r *http.Request, page Page, content templ.Com
 	}
 
 	if err := Base(page, content).Render(r.Context(), w); err != nil {
-		slog.Error("failed to render page", "error", err)
+		if !web.IsClientDisconnect(err) {
+			slog.Error("failed to render page", "error", err)
+		}
 	}
 }
 
 func ServerError(w http.ResponseWriter, msg string, err error) {
-	if errors.Is(err, context.Canceled) {
+	if web.IsClientDisconnect(err) {
 		return
 	}
 	slog.Error(msg, "error", err)

internal/web/error.go

@@ -1,7 +1,9 @@
 package web
 
 import (
+	"context"
 	"encoding/json"
+	"errors"
 	"log/slog"
 	"net/http"
 	"strconv"
@@ -42,6 +44,21 @@ func InternalServerError(w http.ResponseWriter, detail string) {
 	WriteError(w, http.StatusInternalServerError, detail)
 }
 
+// ServerError logs the error and writes a 500 response, unless the error
+// is due to a canceled context (client disconnect), in which case it's a no-op.
+func ServerError(w http.ResponseWriter, msg string, err error) {
+	if errors.Is(err, context.Canceled) {
+		return
+	}
+	slog.Error(msg, "error", err)
+	InternalServerError(w, "internal server error")
+}
+
+// IsClientDisconnect reports whether the error is due to a canceled context.
+func IsClientDisconnect(err error) bool {
+	return errors.Is(err, context.Canceled)
+}
+
 func TooManyRequests(w http.ResponseWriter, detail string, retryAfterSeconds int) {
 	w.Header().Set("Retry-After", strconv.Itoa(retryAfterSeconds))
 	WriteError(w, http.StatusTooManyRequests, detail)

internal/web/error_test.go

@@ -1,7 +1,10 @@
 package web
 
 import (
+	"context"
 	"encoding/json"
+	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -50,4 +53,58 @@ func TestErrorResponses(t *testing.T) {
 			t.Errorf("got Retry-After %s", rr.Header().Get("Retry-After"))
 		}
 	})
+
+	t.Run("ServerError writes 500 for real errors", func(t *testing.T) {
+		rr := httptest.NewRecorder()
+		ServerError(rr, "db failed", errors.New("connection refused"))
+		if rr.Code != http.StatusInternalServerError {
+			t.Errorf("got %d, want %d", rr.Code, http.StatusInternalServerError)
+		}
+		if ct := rr.Header().Get("Content-Type"); ct != "application/problem+json" {
+			t.Errorf("got Content-Type %q", ct)
+		}
+		if cc := rr.Header().Get("Cache-Control"); cc != "no-store" {
+			t.Errorf("got Cache-Control %q", cc)
+		}
+	})
+
+	t.Run("ServerError is no-op for context.Canceled", func(t *testing.T) {
+		rr := httptest.NewRecorder()
+		ServerError(rr, "should not log", context.Canceled)
+		if rr.Code != http.StatusOK {
+			t.Errorf("got %d, want %d (no response written)", rr.Code, http.StatusOK)
+		}
+		if rr.Body.Len() != 0 {
+			t.Errorf("expected empty body, got %q", rr.Body.String())
+		}
+	})
+
+	t.Run("ServerError is no-op for wrapped context.Canceled", func(t *testing.T) {
+		rr := httptest.NewRecorder()
+		ServerError(rr, "should not log", fmt.Errorf("query: %w", context.Canceled))
+		if rr.Code != http.StatusOK {
+			t.Errorf("got %d, want %d (no response written)", rr.Code, http.StatusOK)
+		}
+	})
+}
+
+func TestIsClientDisconnect(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"context.Canceled", context.Canceled, true},
+		{"wrapped context.Canceled", fmt.Errorf("scan: %w", context.Canceled), true},
+		{"generic error", errors.New("timeout"), false},
+		{"nil", nil, false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := IsClientDisconnect(tt.err); got != tt.want {
+				t.Errorf("IsClientDisconnect(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
 }