Stop logging phantom 500s and cap the catalog refresh retries

internal/handlers/home.go

@@ -3,6 +3,7 @@ package handlers
 import (
 	"encoding/json"
 	"errors"
+	"log"
 	"net/http"
 	"strings"
 
@@ -99,11 +100,36 @@ func (h *HomeHandler) render(w http.ResponseWriter, r *http.Request, snap catalo
 	if msg, ok := flash.MessageFromRequest(r); ok {
 		props["flash"] = inertia.Flash{msg.Kind: msg.Message}
 	}
-	if err := h.inertia.Render(w, r, "Home", props); err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+	tracked := &trackedWriter{ResponseWriter: w}
+	if err := h.inertia.Render(tracked, r, "Home", props); err != nil {
+		// Inertia writes the template straight to the response, so a client that
+		// aborts mid-page leaves the status committed. Rewriting it as 500 would
+		// log a phantom server error and append text to the partial HTML.
+		if tracked.started {
+			log.Printf("home: render %s: %v", r.URL.Path, err)
+			return
+		}
+		http.Error(tracked, err.Error(), http.StatusInternalServerError)
 	}
 }
 
+// trackedWriter records whether anything was written, which is the only way to
+// tell a render failure that can still become a 500 from one that cannot.
+type trackedWriter struct {
+	http.ResponseWriter
+	started bool
+}
+
+func (w *trackedWriter) WriteHeader(code int) {
+	w.started = true
+	w.ResponseWriter.WriteHeader(code)
+}
+
+func (w *trackedWriter) Write(p []byte) (int, error) {
+	w.started = true
+	return w.ResponseWriter.Write(p)
+}
+
 func (h *HomeHandler) RepoTraffic(w http.ResponseWriter, r *http.Request, owner, repo string) {
 	owner = normalizeLogin(owner)
 	repo = strings.TrimSpace(repo)

internal/handlers/home_test.go

@@ -2,6 +2,7 @@ package handlers
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -12,6 +13,31 @@ import (
 	"github.com/puppe1990/github-projects-viewer-cais/internal/githubapi"
 )
 
+// deadClientWriter reproduces a client that goes away mid-response: net/http
+// records the implicit 200 when the first body chunk is written, then every
+// write fails because the connection is gone.
+type deadClientWriter struct {
+	header   http.Header
+	statuses []int
+}
+
+func newDeadClientWriter() *deadClientWriter {
+	return &deadClientWriter{header: http.Header{}}
+}
+
+func (w *deadClientWriter) Header() http.Header { return w.header }
+
+func (w *deadClientWriter) WriteHeader(code int) {
+	w.statuses = append(w.statuses, code)
+}
+
+func (w *deadClientWriter) Write(p []byte) (int, error) {
+	if len(w.statuses) == 0 {
+		w.statuses = append(w.statuses, http.StatusOK)
+	}
+	return 0, errors.New("write: connection reset by peer")
+}
+
 type homeGitHub struct {
 	user     githubapi.User
 	repos    []githubapi.Repo
@@ -194,3 +220,12 @@ func TestHomeHandler_ContentType(t *testing.T) {
 		t.Errorf("Content-Type = %q", got)
 	}
 }
+
+func TestHomeHandler_AbortedClientDoesNotRewriteHeader(t *testing.T) {
+	h := newHomeHandler(t, homeGitHub{})
+	w := newDeadClientWriter()
+	h.Index(w, httptest.NewRequest(http.MethodGet, "/", nil))
+	if len(w.statuses) != 1 || w.statuses[0] != http.StatusOK {
+		t.Fatalf("statuses = %v, want [200]: an aborted client already has a response in flight, so appending an error page only corrupts it and logs a phantom 500", w.statuses)
+	}
+}

internal/jobs/rate_limit.go

@@ -0,0 +1,19 @@
+package jobs
+
+import (
+	"errors"
+	"log"
+
+	"github.com/puppe1990/github-projects-viewer-cais/internal/githubapi"
+)
+
+// skipRateLimited ends a rate-limited run without marking it failed. The worker
+// backs off seconds while GitHub's window resets in up to an hour, so retrying
+// now only buries the job in the failed pile; the next scheduled run refreshes it.
+func skipRateLimited(err error) error {
+	if !errors.Is(err, githubapi.ErrRateLimited) {
+		return err
+	}
+	log.Printf("jobs skipped: %v; the hourly cron retries later", err)
+	return nil
+}

internal/jobs/refresh_catalog.go

@@ -18,18 +18,17 @@ func PerformRefreshCatalog(loader *catalog.Loader) caisjobs.Handler {
 		var p refreshPayload
 		_ = json.Unmarshal(payload, &p)
 		if p.Login != "" {
-			return loader.Refresh(ctx, p.Login)
+			return skipRateLimited(loader.Refresh(ctx, p.Login))
 		}
 		logins, err := loader.Cache.WatchedLogins()
 		if err != nil {
 			return err
 		}
-		var first error
 		for _, login := range logins {
-			if err := loader.Refresh(ctx, login); err != nil && first == nil {
-				first = err
+			if err := skipRateLimited(loader.Refresh(ctx, login)); err != nil {
+				return err
 			}
 		}
-		return first
+		return nil
 	}
 }

internal/jobs/snapshot_traffic.go

@@ -3,13 +3,10 @@ package jobs
 import (
 	"context"
 	"encoding/json"
-	"errors"
-	"log"
 
 	caisjobs "github.com/puppe1990/cais/pkg/cais/jobs"
 
 	"github.com/puppe1990/github-projects-viewer-cais/internal/catalog"
-	"github.com/puppe1990/github-projects-viewer-cais/internal/githubapi"
 )
 
 type trafficPayload struct {
@@ -37,22 +34,10 @@ func snapshotTraffic(ctx context.Context, loader *catalog.Loader, p trafficPaylo
 	if err != nil {
 		return err
 	}
-	var first error
 	for _, login := range logins {
-		if err := loader.SnapshotLogin(ctx, login); err != nil && first == nil {
-			first = err
+		if err := loader.SnapshotLogin(ctx, login); err != nil {
+			return err
 		}
 	}
-	return first
-}
-
-// skipRateLimited ends a rate-limited run without marking it failed. The worker
-// backs off seconds while GitHub's window resets in up to an hour, so retrying
-// now only buries the job in the failed pile; the next scheduled run refreshes it.
-func skipRateLimited(err error) error {
-	if !errors.Is(err, githubapi.ErrRateLimited) {
-		return err
-	}
-	log.Printf("jobs skipped: %v; the hourly SnapshotTraffic cron retries later", err)
 	return nil
 }

internal/jobs/snapshot_traffic_test.go

@@ -2,6 +2,7 @@ package jobs
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -14,26 +15,29 @@ type stubGitHub struct {
 	traffic    githubapi.Traffic
 	trafficErr error
 	user       githubapi.User
+	userErr    error
 	repos      []githubapi.Repo
+	userCalls  int
 }
 
-func (s stubGitHub) HasToken() bool { return true }
-func (s stubGitHub) Me(context.Context) (githubapi.User, error) {
+func (s *stubGitHub) HasToken() bool { return true }
+func (s *stubGitHub) Me(context.Context) (githubapi.User, error) {
 	return s.user, nil
 }
-func (s stubGitHub) User(context.Context, string) (githubapi.User, error) {
-	return s.user, nil
+func (s *stubGitHub) User(context.Context, string) (githubapi.User, error) {
+	s.userCalls++
+	return s.user, s.userErr
 }
-func (s stubGitHub) UserRepos(context.Context, string) ([]githubapi.Repo, error) {
+func (s *stubGitHub) UserRepos(context.Context, string) ([]githubapi.Repo, error) {
 	return s.repos, nil
 }
-func (s stubGitHub) UserOrgs(context.Context, string) ([]githubapi.Org, error) {
+func (s *stubGitHub) UserOrgs(context.Context, string) ([]githubapi.Org, error) {
 	return nil, nil
 }
-func (s stubGitHub) OrgRepos(context.Context, string) ([]githubapi.Repo, error) {
+func (s *stubGitHub) OrgRepos(context.Context, string) ([]githubapi.Repo, error) {
 	return nil, nil
 }
-func (s stubGitHub) Traffic(context.Context, string, string) (githubapi.Traffic, error) {
+func (s *stubGitHub) Traffic(context.Context, string, string) (githubapi.Traffic, error) {
 	return s.traffic, s.trafficErr
 }
 
@@ -53,7 +57,7 @@ func testJobLoader(t *testing.T, gh catalog.GitHub) (*catalog.Loader, store.Stor
 }
 
 func TestPerformSnapshotTraffic_SavesWindow(t *testing.T) {
-	loader, s := testJobLoader(t, stubGitHub{
+	loader, s := testJobLoader(t, &stubGitHub{
 		traffic: githubapi.Traffic{Views: 42, Clones: 7, Available: true},
 	})
 	h := PerformSnapshotTraffic(loader)
@@ -67,7 +71,7 @@ func TestPerformSnapshotTraffic_SavesWindow(t *testing.T) {
 }
 
 func TestPerformSnapshotTraffic_LoginUsesCachedRepos(t *testing.T) {
-	loader, s := testJobLoader(t, stubGitHub{
+	loader, s := testJobLoader(t, &stubGitHub{
 		traffic: githubapi.Traffic{Views: 3, Available: true},
 	})
 	if err := s.SaveRepos("octocat", catalog.SourceUser, []catalog.Repo{{Name: "hello-world", OwnerLogin: "octocat"}}, time.Now()); err != nil {
@@ -84,7 +88,7 @@ func TestPerformSnapshotTraffic_LoginUsesCachedRepos(t *testing.T) {
 }
 
 func TestPerformSnapshotTraffic_RateLimitedIsSkipped(t *testing.T) {
-	loader, s := testJobLoader(t, stubGitHub{trafficErr: githubapi.ErrRateLimited})
+	loader, s := testJobLoader(t, &stubGitHub{trafficErr: githubapi.ErrRateLimited})
 	if err := s.SaveRepos("octocat", catalog.SourceUser, []catalog.Repo{{Name: "hello-world", OwnerLogin: "octocat"}}, time.Now()); err != nil {
 		t.Fatal(err)
 	}
@@ -95,7 +99,7 @@ func TestPerformSnapshotTraffic_RateLimitedIsSkipped(t *testing.T) {
 }
 
 func TestPerformRefreshCatalog_FetchesUser(t *testing.T) {
-	loader, s := testJobLoader(t, stubGitHub{
+	loader, s := testJobLoader(t, &stubGitHub{
 		user:  githubapi.User{Login: "octocat", Name: "The Octocat"},
 		repos: []githubapi.Repo{{Name: "hello-world", OwnerLogin: "octocat", Stars: 9}},
 	})
@@ -108,3 +112,31 @@ func TestPerformRefreshCatalog_FetchesUser(t *testing.T) {
 		t.Fatalf("ok=%v err=%v profile=%+v", ok, err, profile)
 	}
 }
+
+func TestPerformRefreshCatalog_StopsOnFirstError(t *testing.T) {
+	gh := &stubGitHub{userErr: errors.New("github: HTTP 502")}
+	loader, s := testJobLoader(t, gh)
+	for _, login := range []string{"alpha", "beta", "gamma"} {
+		if err := s.TouchWatch(login); err != nil {
+			t.Fatal(err)
+		}
+	}
+	h := PerformRefreshCatalog(loader)
+	if err := h(context.Background(), []byte(`{}`)); err == nil {
+		t.Fatal("expected the fetch error to surface")
+	}
+	if gh.userCalls != 1 {
+		t.Fatalf("user calls = %d, want 1: a failing login must not make the job walk the whole watch list", gh.userCalls)
+	}
+}
+
+func TestPerformRefreshCatalog_RateLimitedIsSkipped(t *testing.T) {
+	loader, s := testJobLoader(t, &stubGitHub{userErr: githubapi.ErrRateLimited})
+	if err := s.TouchWatch("octocat"); err != nil {
+		t.Fatal(err)
+	}
+	h := PerformRefreshCatalog(loader)
+	if err := h(context.Background(), []byte(`{}`)); err != nil {
+		t.Fatalf("rate limit must not fail the job: %v", err)
+	}
+}