diff --git a/internal/agent/client_test.go b/internal/agent/client_test.go index a06b34f..20dac83 100644 --- a/internal/agent/client_test.go +++ b/internal/agent/client_test.go @@ -163,6 +163,59 @@ func TestFetchState(t *testing.T) { } } +// A merged PR whose branch was deleted leaves its head commit unreachable, so +// the commit-status endpoint 404s. FetchState must still return the (merged) +// state rather than failing, otherwise the watch loop never sees the merge. +func TestFetchStateToleratesMissingCommit(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `{"number":7,"state":"closed","merged":true,"mergeable":true,"title":"feat","html_url":"u","head":{"sha":"cafebabecafebabe"}}`) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/commits/cafebabecafebabe/status", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = io.WriteString(w, `{"message":"not found"}`) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/issues/7/comments", func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(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} + st, err := FetchState(c, ref, "unkin-agent") + if err != nil { + t.Fatalf("FetchState must tolerate a 404 status for a gone commit: %v", err) + } + if !st.Merged || st.State != "closed" { + t.Errorf("merged/state = %t/%q, want true/closed", st.Merged, st.State) + } + if st.CIStatus != "" { + t.Errorf("CIStatus = %q, want empty (no status for a gone commit)", st.CIStatus) + } +} + +// A non-404 error from the status endpoint is still fatal: only "commit gone" is +// tolerated, not, say, an auth or server failure. +func TestFetchStateFailsOnNon404StatusError(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls/7", func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `{"number":7,"state":"open","merged":false,"mergeable":true,"head":{"sha":"cafebabecafebabe"}}`) + }) + mux.HandleFunc("/api/v1/repos/unkin/repo/commits/cafebabecafebabe/status", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = io.WriteString(w, `{"message":"boom"}`) + }) + 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} + if _, err := FetchState(c, ref, "unkin-agent"); err == nil { + t.Fatal("FetchState should surface a 500 from the status endpoint") + } +} + func TestGiteaAPIError(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("/api/v1/repos/unkin/repo/pulls", func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/agent/gitea.go b/internal/agent/gitea.go index b454391..bf38e86 100644 --- a/internal/agent/gitea.go +++ b/internal/agent/gitea.go @@ -3,12 +3,33 @@ package agent import ( "bytes" "encoding/json" + "errors" "fmt" "io" "net/http" "strings" ) +// APIError is a non-2xx response from the Gitea API. It carries the status code +// so callers can react to specific failures (e.g. tolerate a 404 for a commit +// whose branch was deleted after a merge) instead of parsing error strings. +type APIError struct { + Method string + Path string + StatusCode int + Body string +} + +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 { + var apiErr *APIError + return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound +} + // GiteaClient talks to the Gitea REST API as the agent user. type GiteaClient struct { BaseURL string @@ -49,7 +70,7 @@ func (c *GiteaClient) do(method, path string, body any, out any) error { defer func() { _ = resp.Body.Close() }() data, _ := io.ReadAll(resp.Body) if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return fmt.Errorf("gitea %s %s: HTTP %d: %s", method, path, resp.StatusCode, strings.TrimSpace(string(data))) + return &APIError{Method: method, Path: path, StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(data))} } if out != nil && len(data) > 0 { if err := json.Unmarshal(data, out); err != nil { diff --git a/internal/agent/watch.go b/internal/agent/watch.go index 23aa239..b422928 100644 --- a/internal/agent/watch.go +++ b/internal/agent/watch.go @@ -22,8 +22,11 @@ func FetchState(c *GiteaClient, ref PRRef, agentLogin string) (PRState, error) { if err != nil { 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 { + if err != nil && !isNotFound(err) { return PRState{}, err } comments, err := c.ListComments(ref.RepoPath(), ref.Number) diff --git a/internal/agent/watch_test.go b/internal/agent/watch_test.go index dab44fa..ff6dbf8 100644 --- a/internal/agent/watch_test.go +++ b/internal/agent/watch_test.go @@ -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"}},