Merge pull request 'Abort watchpr when a poll can no longer see the PR' (#12) from benvin/watchpr-terminal-errors into main

Reviewed-on: #12

cmd/watchpr/main.go

@@ -140,6 +140,9 @@ 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.IsPRGone(err) {
+			return fmt.Errorf("PR no longer visible (repo deleted, renamed, or made private), watch aborted: %w", err)
+		}
 		return err
 	}
 	report(res.Ref.String(), res.Reason, res.State, jsonMode)

internal/agent/gitea.go

@@ -24,8 +24,10 @@ func (e *APIError) Error() string {
 	return fmt.Sprintf("gitea %s %s: HTTP %d: %s", e.Method, e.Path, e.StatusCode, e.Body)
 }
 
-// isNotFound reports whether err is a Gitea 404.
-func isNotFound(err error) bool {
+// IsNotFound reports whether err is a Gitea 404. Gitea hides repositories a
+// caller may not see behind a 404 rather than a 403, so this also covers a repo
+// that was renamed, deleted, or made private.
+func IsNotFound(err error) bool {
 	var apiErr *APIError
 	return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
 }

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,13 +34,16 @@ 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/
 	// rebase merge); the PR object is still authoritative, so treat CI as absent
 	// rather than discarding the merge signal and hanging the watch loop.
 	ci, err := c.CommitStatus(ref.RepoPath(), pr.Head.Sha)
-	if err != nil && !isNotFound(err) {
+	if err != nil && !IsNotFound(err) {
 		return PRState{}, err
 	}
 	comments, err := c.ListComments(ref.RepoPath(), ref.Number)
@@ -81,14 +95,22 @@ func terminalState(st PRState) (bool, string) {
 	return false, ""
 }
 
+// MaxPollFailures is how many consecutive failed polls of the same PR are
+// 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
 // tracked PR changes meaningfully, returning the first such change. A PR that is
 // already terminal (merged/closed) at baseline is reported immediately rather
-// than polled forever. Poll errors are handed to onError and never stop the
-// loop; a baseline fetch error and an authentication failure (the token was
-// rejected and re-minting it did not help) abort instead, because a watcher that
-// cannot authenticate sees nothing. onBaseline, if set, fires once after all
-// baselines are captured and before the first tick.
+// 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 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) {
 	prev := make(map[string]PRState, len(refs))
 	for _, ref := range refs {
@@ -104,19 +126,25 @@ func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Ti
 	if onBaseline != nil {
 		onBaseline()
 	}
+	fails := make(map[string]int, len(refs))
 	for range ticks {
 		for _, ref := range refs {
 			key := ref.String()
 			cur, err := f.FetchState(ref, agentLogin)
 			if err != nil {
-				if IsAuthError(err) {
+				if IsAuthError(err) || IsPRGone(err) {
 					return WatchResult{}, fmt.Errorf("polling %s: %w", key, err)
 				}
+				fails[key]++
 				if onError != nil {
 					onError(ref, err)
 				}
+				if fails[key] >= MaxPollFailures {
+					return WatchResult{}, fmt.Errorf("polling %s: giving up after %d consecutive failures: %w", key, fails[key], err)
+				}
 				continue
 			}
+			fails[key] = 0
 			if changed, reason := MeaningfulChange(prev[key], cur); changed {
 				return WatchResult{Ref: ref, Reason: reason, State: cur}, nil
 			}

internal/agent/watch_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -421,6 +422,363 @@ func TestWatchContinuesPastServerError(t *testing.T) {
 	}
 }
 
+// The production failure: a watched repo was renamed mid-watch, so every poll
+// 404'd (Gitea hides a repo the caller may not see rather than 403ing) and the
+// loop warned past it forever while reporting nothing. A 404 on a tracked PR
+// must end the watch with an error naming that PR.
+func TestWatchAbortsOnMidRunNotFound(t *testing.T) {
+	const sha = "deadbeefdeadbeef"
+	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 { // repo renamed/made private after the baseline
+			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()
+	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 abort on a mid-run 404, not keep polling")
+		}
+		if !IsNotFound(err) {
+			t.Errorf("Watch error = %v, want a 404", err)
+		}
+		if !strings.Contains(err.Error(), ref.String()) {
+			t.Errorf("Watch error = %v, want it to name %s", err, ref.String())
+		}
+		if n := warned.Load(); n != 0 {
+			t.Errorf("404 was logged as a warning %d time(s); it must abort", n)
+		}
+	case <-time.After(3 * time.Second):
+		t.Fatal("Watch hung: a vanished repo was warned past instead of aborting")
+	}
+}
+
+// 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) {
+	const sha = "feedfacefeedface"
+	var polls atomic.Int32
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) {
+		switch n := polls.Add(1); {
+		case n == 2 || n == 3: // gateway blip across two polls
+			w.WriteHeader(http.StatusBadGateway)
+			_, _ = fmt.Fprint(w, `bad gateway`)
+		case n >= 4:
+			_, _ = fmt.Fprintf(w, `{"number":7,"state":"closed","merged":true,"mergeable":true,"head":{"sha":%q}}`, sha)
+		default:
+			_, _ = 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()
+	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", 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 transient 5xx must not stop the watch")
+	}
+}
+
+// pollFailures scripts a fetcher whose polls fail with a 502 at the given call
+// indexes (0 is the baseline); the final call returns merged.
+func pollFailures(calls int, failAt map[int]bool) *fakeFetcher {
+	open, merged := base(), base()
+	merged.State = "closed"
+	merged.Merged = true
+	f := &fakeFetcher{states: make([]PRState, calls), errs: make([]error, calls)}
+	for i := range calls {
+		f.states[i] = open
+		if failAt[i] {
+			f.errs[i] = &APIError{Method: "GET", Path: "/p", StatusCode: 502, Body: "bad gateway"}
+		}
+	}
+	f.states[calls-1] = merged
+	return f
+}
+
+// A permanently wedged endpoint (5xx forever) must eventually give up instead of
+// warning on every tick for the life of the process.
+func TestWatchAbortsAfterConsecutiveFailures(t *testing.T) {
+	failAt := map[int]bool{}
+	for i := 1; i <= MaxPollFailures; i++ {
+		failAt[i] = true
+	}
+	f := pollFailures(MaxPollFailures+1, failAt)
+
+	warned := 0
+	ticks := make(chan time.Time, MaxPollFailures)
+	for range MaxPollFailures {
+		ticks <- time.Now()
+	}
+	close(ticks)
+	_, err := Watch(f, []PRRef{base().Ref}, "unkin-agent", ticks, nil, func(PRRef, error) { warned++ })
+	if err == nil {
+		t.Fatal("Watch should give up once the failures stop being transient")
+	}
+	if !strings.Contains(err.Error(), "consecutive failures") {
+		t.Errorf("Watch error = %v, want it to report the failure cap", err)
+	}
+	if warned != MaxPollFailures {
+		t.Errorf("warnings = %d, want %d", warned, MaxPollFailures)
+	}
+	if f.calls != MaxPollFailures+1 {
+		t.Errorf("fetch calls = %d, want %d", f.calls, MaxPollFailures+1)
+	}
+}
+
+// The cap counts consecutive failures only: a single successful poll clears it,
+// so an intermittent endpoint is watched indefinitely and the merge is caught.
+func TestWatchFailureCountResetsOnSuccess(t *testing.T) {
+	const runs = MaxPollFailures - 1
+	failAt := map[int]bool{}
+	for i := 1; i <= runs; i++ { // first run of failures
+		failAt[i] = true
+	}
+	for i := runs + 2; i <= 2*runs+1; i++ { // second run, after one good poll
+		failAt[i] = true
+	}
+	f := pollFailures(2*runs+3, failAt)
+
+	ticks := make(chan time.Time, 2*runs+2)
+	for range 2*runs + 2 {
+		ticks <- time.Now()
+	}
+	close(ticks)
+	res, err := Watch(f, []PRRef{base().Ref}, "unkin-agent", ticks, nil, func(PRRef, error) {})
+	if err != nil {
+		t.Fatalf("Watch: %v (a successful poll must reset the failure count)", err)
+	}
+	if res.Reason != "PR merged" {
+		t.Errorf("reason = %q, want %q", res.Reason, "PR merged")
+	}
+}
+
 // Anonymous watching of a public repo must poll on without a credential in
 // sight: no token, no mint, no exit until something actually changes.
 func TestWatchAnonymousKeepsPolling(t *testing.T) {