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

@@ -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

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]++

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) {