From d04c5aa58dc4b603b4482eb1546e287e0f9e66e2 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Wed, 9 Sep 2026 22:55:56 +1000 Subject: [PATCH] Scope watchpr's terminal 404 to the PR lookup Only a 404 from GetPR means the PR is gone. A 404 from any other call can be a proxy or ingress blip, so it now warns and counts against the consecutive-failure cap instead of killing the watch on first sight. --- cmd/watchpr/main.go | 2 +- internal/agent/watch.go | 27 ++++-- internal/agent/watch_test.go | 165 +++++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 7 deletions(-) diff --git a/cmd/watchpr/main.go b/cmd/watchpr/main.go index 8ba1f4d..e60f312 100644 --- a/cmd/watchpr/main.go +++ b/cmd/watchpr/main.go @@ -135,7 +135,7 @@ func runWatch(c *agent.GiteaClient, refs []agent.PRRef, interval time.Duration, if agent.IsAuthError(err) { return fmt.Errorf("gitea authentication failed after re-minting the token, watch aborted: %w", err) } - if agent.IsNotFound(err) { + if agent.IsPRGone(err) { return fmt.Errorf("PR no longer visible (repo deleted, renamed, or made private), watch aborted: %w", err) } return err diff --git a/internal/agent/watch.go b/internal/agent/watch.go index c194e71..385fa8c 100644 --- a/internal/agent/watch.go +++ b/internal/agent/watch.go @@ -1,10 +1,21 @@ package agent import ( + "errors" "fmt" "time" ) +// errPRGone marks a 404 from the PR lookup itself. A 404 from any other endpoint +// can be a proxy or ingress blip and is left to the ordinary failure cap. +var errPRGone = errors.New("PR no longer visible") + +// IsPRGone reports whether err is a 404 from the PR lookup, meaning the PR is no +// longer visible rather than one endpoint being briefly unreachable. +func IsPRGone(err error) bool { + return errors.Is(err, errPRGone) +} + // PRState is a point-in-time snapshot of the PR attributes watchpr tracks. type PRState struct { Ref PRRef `json:"ref"` @@ -23,6 +34,9 @@ type PRState struct { func FetchState(c *GiteaClient, ref PRRef, agentLogin string) (PRState, error) { pr, err := c.GetPR(ref.RepoPath(), ref.Number) if err != nil { + if IsNotFound(err) { + return PRState{}, fmt.Errorf("%w: %w", errPRGone, err) + } return PRState{}, err } // A 404 here means the head commit is gone (branch deleted after a squash/ @@ -82,8 +96,8 @@ func terminalState(st PRState) (bool, string) { } // MaxPollFailures is how many consecutive failed polls of the same PR are -// tolerated before Watch gives up. At watchpr's default 60s interval that rides -// out a 20-minute outage. +// tolerated before Watch gives up. The abort fires on the 20th failed tick, so +// at watchpr's default 60s interval a watch rides out ~19 minutes of failure. const MaxPollFailures = 20 // Watch establishes a baseline for each ref, then polls on every tick until a @@ -91,9 +105,10 @@ const MaxPollFailures = 20 // already terminal (merged/closed) at baseline is reported immediately rather // than polled forever. Transient poll errors are handed to onError and the loop // continues, but never blindly: a baseline fetch error, an authentication -// failure surviving a token re-mint, a 404 on a tracked PR (the repo is gone, -// renamed, or no longer visible), and MaxPollFailures consecutive failures of -// one PR all abort, because a watcher that sees nothing must not look healthy. +// failure surviving a token re-mint, a 404 from the PR lookup itself (the repo +// is gone, renamed, or no longer visible), and MaxPollFailures consecutive +// failures of one PR all abort, because a watcher that sees nothing must not +// look healthy. // onBaseline, if set, fires once after all baselines are captured and before the // first tick. func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Time, onBaseline func(), onError func(PRRef, error)) (WatchResult, error) { @@ -117,7 +132,7 @@ func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Ti key := ref.String() cur, err := f.FetchState(ref, agentLogin) if err != nil { - if IsAuthError(err) || IsNotFound(err) { + if IsAuthError(err) || IsPRGone(err) { return WatchResult{}, fmt.Errorf("polling %s: %w", key, err) } fails[key]++ diff --git a/internal/agent/watch_test.go b/internal/agent/watch_test.go index 6230920..101ecff 100644 --- a/internal/agent/watch_test.go +++ b/internal/agent/watch_test.go @@ -480,6 +480,171 @@ func TestWatchAbortsOnMidRunNotFound(t *testing.T) { } } +// A 404 from a sub-resource is not proof the PR is gone: an ingress can serve +// one during a Gitea rolling restart. Only the PR lookup itself is authoritative, +// so a comments 404 must warn and keep polling like any other transient failure, +// and still catch the merge that lands afterwards. +func TestWatchSurvivesCommentsNotFound(t *testing.T) { + const sha = "0badc0de0badc0de" + var polls atomic.Int32 + + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + if polls.Add(1) >= 4 { + _, _ = fmt.Fprintf(w, `{"number":7,"state":"closed","merged":true,"mergeable":true,"head":{"sha":%q}}`, sha) + return + } + _, _ = fmt.Fprintf(w, `{"number":7,"state":"open","merged":false,"mergeable":true,"head":{"sha":%q}}`, sha) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/commits/"+sha+"/status", func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprint(w, `{"state":"success"}`) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/issues/7/comments", func(w http.ResponseWriter, r *http.Request) { + if n := polls.Load(); n == 2 || n == 3 { // proxy blip across two polls + w.WriteHeader(http.StatusNotFound) + _, _ = fmt.Fprint(w, `{"message":"Not Found"}`) + return + } + _, _ = fmt.Fprint(w, `[]`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()} + ref := PRRef{Owner: "unkin", Repo: "repo", Number: 7} + + tk := time.NewTicker(5 * time.Millisecond) + defer tk.Stop() + var warned atomic.Int32 + type outcome struct { + res WatchResult + err error + } + done := make(chan outcome, 1) + go func() { + res, err := Watch(c, []PRRef{ref}, "unkin-agent", tk.C, nil, + func(PRRef, error) { warned.Add(1) }) + done <- outcome{res, err} + }() + + select { + case o := <-done: + if o.err != nil { + t.Fatalf("Watch: %v (a comments 404 must not be terminal)", o.err) + } + if o.res.Reason != "PR merged" { + t.Errorf("reason = %q, want %q", o.res.Reason, "PR merged") + } + if n := warned.Load(); n != 2 { + t.Errorf("warnings = %d, want 2", n) + } + case <-time.After(3 * time.Second): + t.Fatal("Watch hung: a comments 404 must warn and keep polling") + } +} + +// A comments 404 costs a poll from the same budget as any other failure: it must +// not be free, and a permanently 404ing sub-resource must still end the watch. +func TestWatchCommentsNotFoundCountsTowardCap(t *testing.T) { + const sha = "1badc0de1badc0de" + + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprintf(w, `{"number":7,"state":"open","merged":false,"mergeable":true,"head":{"sha":%q}}`, sha) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/commits/"+sha+"/status", func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprint(w, `{"state":"success"}`) + }) + var comments atomic.Int32 + mux.HandleFunc("/api/v1/repos/unkin/repo/issues/7/comments", func(w http.ResponseWriter, r *http.Request) { + if comments.Add(1) > 1 { // healthy at baseline, gone from the first poll on + w.WriteHeader(http.StatusNotFound) + _, _ = fmt.Fprint(w, `{"message":"Not Found"}`) + return + } + _, _ = fmt.Fprint(w, `[]`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()} + ref := PRRef{Owner: "unkin", Repo: "repo", Number: 7} + + tk := time.NewTicker(time.Millisecond) + defer tk.Stop() + var warned atomic.Int32 + done := make(chan error, 1) + go func() { + _, err := Watch(c, []PRRef{ref}, "unkin-agent", tk.C, nil, + func(PRRef, error) { warned.Add(1) }) + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("Watch should give up once the comments 404 stops being transient") + } + if !strings.Contains(err.Error(), "consecutive failures") { + t.Errorf("Watch error = %v, want it to report the failure cap", err) + } + if n := warned.Load(); n != MaxPollFailures { + t.Errorf("warnings = %d, want %d", n, MaxPollFailures) + } + case <-time.After(3 * time.Second): + t.Fatal("Watch hung: a permanently 404ing comments endpoint must hit the cap") + } +} + +// The PR lookup is the call whose 404 means the PR is gone, so it aborts on the +// very first occurrence rather than spending the failure budget. +func TestWatchAbortsOnFirstPRLookupNotFound(t *testing.T) { + const sha = "2badc0de2badc0de" + var polls atomic.Int32 + + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + if polls.Add(1) > 1 { + w.WriteHeader(http.StatusNotFound) + _, _ = fmt.Fprint(w, `{"message":"Not Found"}`) + return + } + _, _ = fmt.Fprintf(w, `{"number":7,"state":"open","merged":false,"mergeable":true,"head":{"sha":%q}}`, sha) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/commits/"+sha+"/status", func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprint(w, `{"state":"success"}`) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/issues/7/comments", func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprint(w, `[]`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()} + ref := PRRef{Owner: "unkin", Repo: "repo", Number: 7} + + tk := time.NewTicker(5 * time.Millisecond) + defer tk.Stop() + done := make(chan error, 1) + go func() { + _, err := Watch(c, []PRRef{ref}, "unkin-agent", tk.C, nil, + func(PRRef, error) { t.Errorf("a PR-lookup 404 must abort, not warn") }) + done <- err + }() + + select { + case err := <-done: + if !IsNotFound(err) { + t.Fatalf("Watch error = %v, want a 404", err) + } + if n := polls.Load(); n != 2 { + t.Errorf("PR fetches = %d, want 2 (baseline + the 404 that aborts)", n) + } + case <-time.After(3 * time.Second): + t.Fatal("Watch hung: a vanished PR must abort") + } +} + // A 5xx blip must not kill a long watch: it warns, keeps polling, and still // catches the merge that lands afterwards. func TestWatchSurvivesTransientServerError(t *testing.T) {