watchpr: fix watch mode hanging when a merged PR's head commit is gone
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/pre-commit Pipeline was successful

FetchState fetched the PR (merged=true) but then failed the whole state
fetch when CommitStatus 404'd for a head commit that no longer existed
(the branch was deleted after a squash/rebase merge). The merge signal was
discarded, so the watch loop treated every post-merge poll as a transient
error and never exited -- the 37-minute hang seen in production.

- Add a typed APIError carrying the HTTP status so callers can detect a 404
  without parsing error strings.
- FetchState now tolerates a 404 from CommitStatus (commit gone => no CI
  status) and returns the authoritative merged/closed PR state.
- Regression tests: FetchState survives a 404 status; the full watch loop,
  driven through a real client, detects a merge whose head commit is gone
  (both fail/hang before the fix).
This commit is contained in:
2026-08-15 13:23:25 +10:00
parent c6712063bc
commit fda3761ead
4 changed files with 155 additions and 2 deletions
+76
View File
@@ -2,6 +2,10 @@ package agent
import (
"errors"
"fmt"
"net/http"
"net/http/httptest"
"sync/atomic"
"testing"
"time"
)
@@ -273,6 +277,78 @@ func TestWatchBaselineErrorAborts(t *testing.T) {
}
}
// The production hang, end to end: a watched PR stays open across several polls,
// then is squash-merged and its branch deleted, so the commit-status endpoint
// 404s. Driven through a real *GiteaClient, the watch loop must still detect the
// merge on the poll it happens. Before the fix, FetchState returned an error on
// that poll (the 404 masked the merge), so the loop reported only poll errors
// and never exited -- exactly the 37-minute hang seen in production.
func TestWatchDetectsMergeWhenCommitGone(t *testing.T) {
const sha = "cafebabecafebabe"
var polls atomic.Int32 // number of PR fetches so far
mux := http.NewServeMux()
mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) {
n := polls.Add(1)
if n >= 4 { // baseline + two unchanged polls, then merged
_, _ = 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) {
if polls.Load() >= 4 { // branch deleted post-merge: commit is gone
w.WriteHeader(http.StatusNotFound)
_, _ = fmt.Fprint(w, `{"message":"not found"}`)
return
}
_, _ = 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}
// A real ticker so the loop advances on its own; a hang (the bug) is caught
// by the timeout below instead of blocking the suite.
tk := time.NewTicker(5 * time.Millisecond)
defer tk.Stop()
var pollErr atomic.Pointer[error]
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, e error) { pollErr.Store(&e) })
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 p := pollErr.Load(); p != nil {
t.Errorf("no poll error expected once a 404 status is tolerated, got: %v", *p)
}
case <-time.After(3 * time.Second):
var got error
if p := pollErr.Load(); p != nil {
got = *p
}
t.Fatalf("Watch hung: a merge with a gone head commit was never detected (last poll error: %v)", got)
}
}
func TestCountNonAgentComments(t *testing.T) {
comments := []Comment{
{User: User{Login: "unkin-agent"}},