diff --git a/internal/handlers/home.go b/internal/handlers/home.go index b627328..fdb312d 100644 --- a/internal/handlers/home.go +++ b/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) diff --git a/internal/handlers/home_test.go b/internal/handlers/home_test.go index bc951bd..ec29ade 100644 --- a/internal/handlers/home_test.go +++ b/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) + } +} diff --git a/internal/jobs/rate_limit.go b/internal/jobs/rate_limit.go new file mode 100644 index 0000000..48bcca6 --- /dev/null +++ b/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 +} diff --git a/internal/jobs/refresh_catalog.go b/internal/jobs/refresh_catalog.go index 29ed978..4a1e161 100644 --- a/internal/jobs/refresh_catalog.go +++ b/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 } } diff --git a/internal/jobs/snapshot_traffic.go b/internal/jobs/snapshot_traffic.go index bd98123..83e373b 100644 --- a/internal/jobs/snapshot_traffic.go +++ b/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 } diff --git a/internal/jobs/snapshot_traffic_test.go b/internal/jobs/snapshot_traffic_test.go index 6d75690..94c4b60 100644 --- a/internal/jobs/snapshot_traffic_test.go +++ b/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) + } +}