From e4f2172e86b1d02376710d708414da53a9358aa0 Mon Sep 17 00:00:00 2001 From: Eric Dahl Date: Mon, 28 Sep 2026 23:01:40 -0400 Subject: [PATCH] refactor: give stuck alerts their own module evaluateStuck and the stuck rules move out of the poller into alerts.Watcher: Observe(repos, now) returns the alerts that fire this cycle. It sees only Snapshots and the clock, so its tests drop the webhook server and hand-built Poller. Also fixes, while the policy moved: - collapsing a Repo no longer reads as a PR recovering, so it no longer restarts the stuck clock - webhooks are sent in the background; a slow endpoint used to block the poll cycle for up to 10s - webhook failures log through logx instead of log.Printf, which wrote over the full-screen dashboard Closes #53 --- CONTEXT.md | 15 ++ internal/alerts/alerts.go | 143 +++++++++++++ internal/alerts/alerts_test.go | 174 ++++++++++++++++ internal/poller/poller.go | 154 +------------- internal/poller/poller_test.go | 359 +++++---------------------------- internal/ui/manage.go | 4 +- internal/webhooks/webhooks.go | 47 +++-- 7 files changed, 424 insertions(+), 472 deletions(-) create mode 100644 internal/alerts/alerts.go create mode 100644 internal/alerts/alerts_test.go diff --git a/CONTEXT.md b/CONTEXT.md index 52afb95..cb25f7c 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -143,6 +143,21 @@ skipped by **Pacing** is not stale — nothing failed, it is simply not due yet. _Avoid_: refresh, sync, watch +## Stuck alert + +A webhook POST (`branch_stuck` or `pr_stuck`) sent when a Repo's branch or one +of its PRs has stayed bad for the stuck threshold (`stuck_threshold_minutes`, +default 30). Bad means a 🔴 Run (`prolonged_failure`), a 🟡 Run +(`prolonged_in_progress`), or, for a PR, a merge conflict (`conflict`). + +- Fires once per incident. Recovering re-arms it, and a changed reason is a + new incident with a fresh clock. +- A collapsed Repo fetches no PR Runs; that is not a recovery, so the clock + keeps running. +- Delivered in the background: a slow endpoint never holds up polling. + +_Avoid_: notification, stuck event + ## Pacing The adjustment of poll frequency to a token's remaining GitHub REST budget. diff --git a/internal/alerts/alerts.go b/internal/alerts/alerts.go new file mode 100644 index 0000000..6481a28 --- /dev/null +++ b/internal/alerts/alerts.go @@ -0,0 +1,143 @@ +// Package alerts decides when a Repo's branch or one of its PRs has been stuck +// long enough to alert on. It sees only Snapshots and the time, so it knows +// nothing about how they were fetched or how an alert is delivered. +package alerts + +import ( + "fmt" + "time" + + "github.com/ericdahl-dev/git-green/internal/aggregator" + githubclient "github.com/ericdahl-dev/git-green/internal/github" + "github.com/ericdahl-dev/git-green/internal/state" + "github.com/ericdahl-dev/git-green/internal/webhooks" +) + +// Watcher tracks stuck conditions across poll cycles. A condition alerts +// exactly once, on the first cycle where it has been bad for at least the +// threshold; recovering forgets it, so the next incident re-arms. +// +// A Watcher is not safe for concurrent use; the Poller calls it once per +// cycle from its fetch loop. +type Watcher struct { + threshold time.Duration + stuck map[string]*entry // keyed by Repo + scope (branch, or PR number) +} + +// entry records when a condition was first seen and whether it has alerted. +type entry struct { + since time.Time + reason string + alerted bool +} + +// NewWatcher returns a Watcher that alerts once a condition has lasted +// threshold. +func NewWatcher(threshold time.Duration) *Watcher { + return &Watcher{threshold: threshold, stuck: make(map[string]*entry)} +} + +// SetThreshold changes the threshold for conditions already being tracked as +// well as new ones, as a config reload would. +func (w *Watcher) SetThreshold(threshold time.Duration) { w.threshold = threshold } + +// Observe folds one poll cycle's Repos into the stuck bookkeeping and returns +// the alerts that fire this cycle. +func (w *Watcher) Observe(repos []state.RepoState, now time.Time) []webhooks.Event { + seen := make(map[string]bool, len(w.stuck)) + var events []webhooks.Event + + // track records one stuck condition and appends an alert if this is the + // cycle that crosses the threshold. + track := func(key, reason string, build func(since time.Time) webhooks.Event) { + seen[key] = true + e, ok := w.stuck[key] + // A changed reason (an in-progress run turning into a failure) is a new + // condition, so restart the clock and allow a fresh alert. + if !ok || e.reason != reason { + e = &entry{since: now, reason: reason} + w.stuck[key] = e + } + if e.alerted || now.Sub(e.since) < w.threshold { + return + } + e.alerted = true + events = append(events, build(e.since)) + } + + for _, r := range repos { + if stuck, reason := runsStuckReason(r.Runs); stuck { + track(key(r, "branch"), reason, func(since time.Time) webhooks.Event { + return event("branch_stuck", reason, r, nil, r.Runs, since, now) + }) + } + + for _, pr := range r.PRs { + k := key(r, fmt.Sprintf("pr-%d", pr.Number)) + stuck, reason := prStuckReason(pr) + if !stuck && len(pr.Runs) == 0 { + // A collapsed Repo fetches no PR Runs. That says nothing about + // the PR, so keep any clock already running rather than reading + // it as a recovery. + seen[k] = true + continue + } + if !stuck { + continue + } + info := &webhooks.PRInfo{Number: pr.Number, Title: pr.Title, URL: pr.HTMLURL} + track(k, reason, func(since time.Time) webhooks.Event { + return event("pr_stuck", reason, r, info, pr.Runs, since, now) + }) + } + } + + // Conditions that recovered stop being tracked, which re-arms them. + for k := range w.stuck { + if !seen[k] { + delete(w.stuck, k) + } + } + return events +} + +func key(r state.RepoState, scope string) string { return r.FullName() + "#" + scope } + +func event(kind, reason string, r state.RepoState, pr *webhooks.PRInfo, runs []githubclient.WorkflowRun, since, now time.Time) webhooks.Event { + evt := webhooks.Event{ + Event: kind, + Reason: reason, + Repo: r.FullName(), + PR: pr, + StuckSince: since, + Timestamp: now, + } + if len(runs) > 0 { + evt.RunURL = runs[0].HTMLURL + evt.Workflow = runs[0].WorkflowName + } + return evt +} + +// prStuckReason reports whether a PR is stuck and why: a merge conflict, or a +// Run that is failing or still going. +func prStuckReason(pr state.PRState) (bool, string) { + if pr.Mergeable == "dirty" || pr.Mergeable == "conflicting" { + return true, "conflict" + } + return runsStuckReason(pr.Runs) +} + +// runsStuckReason reports the first Run that is failing or still going, which +// is what "stuck" means once it has lasted past the threshold. +func runsStuckReason(runs []githubclient.WorkflowRun) (bool, string) { + for _, run := range runs { + switch aggregator.Of(run.Effective()) { + case aggregator.StoplightRed: + return true, "prolonged_failure" + case aggregator.StoplightYellow: + return true, "prolonged_in_progress" + } + } + return false, "" +} diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go new file mode 100644 index 0000000..7c347c0 --- /dev/null +++ b/internal/alerts/alerts_test.go @@ -0,0 +1,174 @@ +package alerts + +import ( + "testing" + "time" + + githubclient "github.com/ericdahl-dev/git-green/internal/github" + "github.com/ericdahl-dev/git-green/internal/state" + "github.com/ericdahl-dev/git-green/internal/webhooks" +) + +var start = time.Date(2026, 8, 26, 9, 0, 0, 0, time.UTC) + +// run feeds the same repos to w every step for n steps and collects alerts. +func run(w *Watcher, repos []state.RepoState, now *time.Time, n int, step time.Duration) []webhooks.Event { + var out []webhooks.Event + for i := 0; i < n; i++ { + out = append(out, w.Observe(repos, *now)...) + *now = now.Add(step) + } + return out +} + +func repoWith(runs ...githubclient.WorkflowRun) []state.RepoState { + return []state.RepoState{{Owner: "o", Name: "r", Runs: runs}} +} + +var ( + failing = repoWith(githubclient.WorkflowRun{WorkflowName: "CI", Status: "completed", Conclusion: "failure"}) + inProgress = repoWith(githubclient.WorkflowRun{WorkflowName: "CI", Status: "in_progress"}) + green = repoWith(githubclient.WorkflowRun{WorkflowName: "CI", Status: "completed", Conclusion: "success"}) +) + +func TestDoesNotFireBeforeThreshold(t *testing.T) { + now := start + if got := run(NewWatcher(30*time.Minute), failing, &now, 80, 15*time.Second); len(got) != 0 { + t.Fatalf("expected no alerts in 20 minutes, got %d", len(got)) + } +} + +func TestFiresOnceAfterThreshold(t *testing.T) { + now := start + got := run(NewWatcher(30*time.Minute), failing, &now, 240, 30*time.Second) + if len(got) != 1 { + t.Fatalf("expected exactly 1 alert over two hours, got %d", len(got)) + } + evt := got[0] + if evt.Event != "branch_stuck" || evt.Reason != "prolonged_failure" || evt.Repo != "o/r" { + t.Errorf("got %q/%q/%q", evt.Event, evt.Reason, evt.Repo) + } + // StuckSince is when the condition started, not when it alerted. + if !evt.StuckSince.Equal(start) { + t.Errorf("stuck_since = %v, want %v", evt.StuckSince, start) + } + if evt.Timestamp.Sub(evt.StuckSince) < 30*time.Minute { + t.Errorf("fired after %v, want at least 30m", evt.Timestamp.Sub(evt.StuckSince)) + } +} + +func TestReArmsAfterRecovery(t *testing.T) { + w := NewWatcher(30 * time.Minute) + now := start + if got := run(w, failing, &now, 5, 10*time.Minute); len(got) != 1 { + t.Fatalf("first incident: got %d alerts, want 1", len(got)) + } + run(w, green, &now, 1, 10*time.Minute) + if len(w.stuck) != 0 { + t.Fatalf("recovery should forget the condition, still tracking %d", len(w.stuck)) + } + if got := run(w, failing, &now, 5, 10*time.Minute); len(got) != 1 { + t.Fatalf("second incident: got %d alerts, want 1", len(got)) + } +} + +func TestReasonChangeRestartsTheClock(t *testing.T) { + w := NewWatcher(30 * time.Minute) + now := start + if got := run(w, inProgress, &now, 2, 10*time.Minute); len(got) != 0 { + t.Fatalf("expected nothing yet, got %d", len(got)) + } + // Turning into a failure is a new condition: the 20 minutes in progress + // must not count toward it. + if got := w.Observe(failing, now); len(got) != 0 { + t.Fatalf("reason change should restart the clock, got %d", len(got)) + } + got := w.Observe(failing, now.Add(31*time.Minute)) + if len(got) != 1 || got[0].Reason != "prolonged_failure" { + t.Fatalf("got %+v, want one prolonged_failure", got) + } +} + +func TestPRFiresWithPRInfo(t *testing.T) { + repos := []state.RepoState{{Owner: "o", Name: "r", PRs: []state.PRState{{ + Number: 42, Title: "Add a thing", HTMLURL: "https://github.com/o/r/pull/42", Mergeable: "dirty", + }}}} + now := start + got := run(NewWatcher(30*time.Minute), repos, &now, 5, 10*time.Minute) + if len(got) != 1 { + t.Fatalf("got %d alerts, want 1", len(got)) + } + if got[0].Event != "pr_stuck" || got[0].Reason != "conflict" || got[0].PR == nil || got[0].PR.Number != 42 { + t.Errorf("got %+v", got[0]) + } +} + +// Collapsing a Repo drops its PR Runs from the Snapshot. That must not read as +// a recovery: the clock keeps running and the alert still fires on time. +func TestCollapsedRepoKeepsThePRClock(t *testing.T) { + failingPR := []state.RepoState{{Owner: "o", Name: "r", PRs: []state.PRState{{ + Number: 7, Runs: []githubclient.WorkflowRun{{WorkflowName: "CI", Status: "completed", Conclusion: "failure"}}, + }}}} + collapsed := []state.RepoState{{Owner: "o", Name: "r", PRs: []state.PRState{{Number: 7}}}} + + w := NewWatcher(30 * time.Minute) + now := start + run(w, failingPR, &now, 2, 10*time.Minute) // 20 minutes failing + run(w, collapsed, &now, 1, 5*time.Minute) // collapsed for 5 + got := run(w, failingPR, &now, 1, 0) // 25 minutes in: still under + if len(got) != 0 { + t.Fatalf("fired early: %+v", got) + } + now = now.Add(6 * time.Minute) + if got := w.Observe(failingPR, now); len(got) != 1 { + t.Fatalf("got %d alerts at 31 minutes, want 1: collapsing reset the clock", len(got)) + } +} + +// A PR that closes is forgotten even though it has no Runs either. +func TestClosedPRIsForgotten(t *testing.T) { + failingPR := []state.RepoState{{Owner: "o", Name: "r", PRs: []state.PRState{{ + Number: 7, Runs: []githubclient.WorkflowRun{{Status: "completed", Conclusion: "failure"}}, + }}}} + w := NewWatcher(30 * time.Minute) + w.Observe(failingPR, start) + w.Observe([]state.RepoState{{Owner: "o", Name: "r"}}, start.Add(time.Minute)) + if len(w.stuck) != 0 { + t.Errorf("a closed PR is still tracked: %d entries", len(w.stuck)) + } +} + +// Stuck follows the Stoplight: every red conclusion is a failure and every +// yellow status is still going. +func TestRunsStuckReasonFollowsStoplight(t *testing.T) { + cases := []struct { + run githubclient.WorkflowRun + stuck bool + reason string + }{ + {githubclient.WorkflowRun{Status: "completed", Conclusion: "failure"}, true, "prolonged_failure"}, + {githubclient.WorkflowRun{Status: "completed", Conclusion: "action_required"}, true, "prolonged_failure"}, + {githubclient.WorkflowRun{Status: "completed", Conclusion: "startup_failure"}, true, "prolonged_failure"}, + {githubclient.WorkflowRun{Status: "in_progress"}, true, "prolonged_in_progress"}, + {githubclient.WorkflowRun{Status: "queued"}, true, "prolonged_in_progress"}, + {githubclient.WorkflowRun{Status: "completed", Conclusion: "success"}, false, ""}, + {githubclient.WorkflowRun{Status: "completed", Conclusion: "cancelled"}, false, ""}, + } + for _, tc := range cases { + stuck, reason := runsStuckReason([]githubclient.WorkflowRun{tc.run}) + if stuck != tc.stuck || reason != tc.reason { + t.Errorf("%s/%s: got (%v, %q), want (%v, %q)", tc.run.Status, tc.run.Conclusion, stuck, reason, tc.stuck, tc.reason) + } + } +} + +func TestPRStuckReasonConflict(t *testing.T) { + for _, m := range []string{"dirty", "conflicting"} { + if stuck, reason := prStuckReason(state.PRState{Mergeable: m}); !stuck || reason != "conflict" { + t.Errorf("%s: got (%v, %q), want conflict", m, stuck, reason) + } + } + if stuck, _ := prStuckReason(state.PRState{Mergeable: "clean"}); stuck { + t.Error("a clean PR with no Runs is not stuck") + } +} diff --git a/internal/poller/poller.go b/internal/poller/poller.go index 2f6b307..e31f1a9 100644 --- a/internal/poller/poller.go +++ b/internal/poller/poller.go @@ -7,6 +7,7 @@ import ( "time" "github.com/ericdahl-dev/git-green/internal/aggregator" + "github.com/ericdahl-dev/git-green/internal/alerts" "github.com/ericdahl-dev/git-green/internal/config" githubclient "github.com/ericdahl-dev/git-green/internal/github" "github.com/ericdahl-dev/git-green/internal/logx" @@ -23,15 +24,6 @@ type Fetcher interface { // ClientFactory creates a Fetcher for a given token. type ClientFactory func(token string) Fetcher -// stuckEntry records when a single condition was first seen in a bad state and -// whether its webhook has already fired, so a wedged repo alerts once rather -// than on every poll cycle. -type stuckEntry struct { - since time.Time - reason string - alerted bool -} - // Poller orchestrates periodic fetches across all configured repos. type Poller struct { cfg *config.Config @@ -39,8 +31,9 @@ type Poller struct { dispatcher *webhooks.Dispatcher mu sync.Mutex current []state.RepoState - // stuck is keyed by repo + condition (see stuckKey) and guarded by mu. - stuck map[string]*stuckEntry + // alerts decides when a branch or PR has been stuck long enough to send + // a webhook. Only the fetch loop touches it. + alerts *alerts.Watcher // expanded holds the "owner/name" of repos whose rows are open in the UI. // Only those fetch per-run jobs and per-PR runs; see RepoQuery.Detail. expanded map[string]bool @@ -77,7 +70,7 @@ func New(cfg *config.Config, factory ClientFactory) *Poller { factory: factory, dispatcher: webhooks.New(cfg.Webhooks), current: repos, - stuck: make(map[string]*stuckEntry), + alerts: alerts.NewWatcher(stuckThreshold(cfg)), expanded: make(map[string]bool), now: time.Now, refresh: make(chan struct{}, 1), @@ -173,9 +166,14 @@ func (p *Poller) applyPending() bool { } p.cfg, p.pending = p.pending, nil p.dispatcher = webhooks.New(p.cfg.Webhooks) + p.alerts.SetThreshold(stuckThreshold(p.cfg)) return true } +func stuckThreshold(cfg *config.Config) time.Duration { + return time.Duration(cfg.Settings.StuckThresholdMinutes) * time.Minute +} + func (p *Poller) interval() time.Duration { return time.Duration(p.cfg.Settings.PollInterval) * time.Second } @@ -230,14 +228,9 @@ func (p *Poller) fetch(ctx context.Context) state.Snapshot { p.mu.Lock() p.current = results - // Compute events under the lock (evaluateStuck mutates p.stuck), but POST - // them after releasing it so a hanging endpoint cannot stall Snapshot(). - events := p.evaluateStuck(results) p.mu.Unlock() - for _, evt := range events { - p.dispatcher.Dispatch(evt) - } + p.dispatcher.Send(p.alerts.Observe(results, p.now())) snap := state.New(results) snap.Throttles = p.Throttles() @@ -335,128 +328,3 @@ func (p *Poller) fetchRepo(ctx context.Context, repo config.Repo, prev state.Rep Err: nil, } } - -// stuckKey identifies one stuck condition: a repo's default branch, or one of -// its open PRs. -func stuckKey(r state.RepoState, scope string) string { - return r.FullName() + "#" + scope -} - -// evaluateStuck folds the freshly-polled state into the stuck bookkeeping and -// returns the events that should fire this cycle. -// -// A condition alerts exactly once, on the first cycle where it has been bad for -// at least the configured threshold. Recovering prunes the entry, so the next -// incident re-arms. Callers must hold p.mu. -func (p *Poller) evaluateStuck(repos []state.RepoState) []webhooks.Event { - threshold := time.Duration(p.cfg.Settings.StuckThresholdMinutes) * time.Minute - now := p.now() - seen := make(map[string]struct{}, len(p.stuck)) - var events []webhooks.Event - - // track records one stuck condition and appends an event if this is the - // cycle that crosses the threshold. - track := func(key, reason string, build func(since time.Time) webhooks.Event) { - seen[key] = struct{}{} - entry, ok := p.stuck[key] - // A changed reason (an in-progress run turning into a failure) is a new - // condition, so restart the clock and allow a fresh alert. - if !ok || entry.reason != reason { - entry = &stuckEntry{since: now, reason: reason} - p.stuck[key] = entry - } - if entry.alerted || now.Sub(entry.since) < threshold { - return - } - entry.alerted = true - events = append(events, build(entry.since)) - } - - for i := range repos { - r := repos[i] - - if stuck, reason := p.branchStuckReason(&r); stuck { - track(stuckKey(r, "branch"), reason, func(since time.Time) webhooks.Event { - runURL, workflow := "", "" - if len(r.Runs) > 0 { - runURL = r.Runs[0].HTMLURL - workflow = r.Runs[0].WorkflowName - } - return webhooks.Event{ - Event: "branch_stuck", - Reason: reason, - Repo: r.FullName(), - Workflow: workflow, - RunURL: runURL, - StuckSince: since, - Timestamp: now, - } - }) - } - - for j := range r.PRs { - pr := r.PRs[j] - stuck, reason := p.prStuckReason(&pr) - if !stuck { - continue - } - track(stuckKey(r, fmt.Sprintf("pr-%d", pr.Number)), reason, func(since time.Time) webhooks.Event { - runURL, workflow := "", "" - if len(pr.Runs) > 0 { - runURL = pr.Runs[0].HTMLURL - workflow = pr.Runs[0].WorkflowName - } - return webhooks.Event{ - Event: "pr_stuck", - Reason: reason, - Repo: r.FullName(), - PR: &webhooks.PRInfo{ - Number: pr.Number, - Title: pr.Title, - URL: pr.HTMLURL, - }, - Workflow: workflow, - RunURL: runURL, - StuckSince: since, - Timestamp: now, - } - }) - } - } - - // Conditions that recovered stop being tracked, which re-arms them. - for key := range p.stuck { - if _, ok := seen[key]; !ok { - delete(p.stuck, key) - } - } - - return events -} - -// branchStuckReason returns whether the branch is stuck and why. -func (p *Poller) branchStuckReason(rs *state.RepoState) (bool, string) { - return runsStuckReason(rs.Runs) -} - -// prStuckReason returns whether the PR is stuck and why. -func (p *Poller) prStuckReason(pr *state.PRState) (bool, string) { - if pr.Mergeable == "dirty" || pr.Mergeable == "conflicting" { - return true, "conflict" - } - return runsStuckReason(pr.Runs) -} - -// runsStuckReason reports the first Run that is failing or still going, which -// is what "stuck" means once it has lasted past the threshold. -func runsStuckReason(runs []githubclient.WorkflowRun) (bool, string) { - for _, run := range runs { - switch aggregator.Of(run.Effective()) { - case aggregator.StoplightRed: - return true, "prolonged_failure" - case aggregator.StoplightYellow: - return true, "prolonged_in_progress" - } - } - return false, "" -} diff --git a/internal/poller/poller_test.go b/internal/poller/poller_test.go index 82598ea..284698c 100644 --- a/internal/poller/poller_test.go +++ b/internal/poller/poller_test.go @@ -166,298 +166,6 @@ name = "git-green" } } -func TestBranchStuckReasonFailure(t *testing.T) { - cfg := &config.Config{} - cfg.Settings.StuckThresholdMinutes = 30 - p := &Poller{cfg: cfg, dispatcher: webhooks.New(nil)} - - rs := &state.RepoState{ - Runs: []githubclient.WorkflowRun{ - {WorkflowName: "CI", Status: "completed", Conclusion: "failure"}, - }, - } - stuck, reason := p.branchStuckReason(rs) - if !stuck { - t.Error("expected stuck=true for failure conclusion") - } - if reason != "prolonged_failure" { - t.Errorf("expected prolonged_failure, got %q", reason) - } -} - -func TestBranchStuckReasonInProgress(t *testing.T) { - cfg := &config.Config{} - cfg.Settings.StuckThresholdMinutes = 30 - p := &Poller{cfg: cfg, dispatcher: webhooks.New(nil)} - - rs := &state.RepoState{ - Runs: []githubclient.WorkflowRun{ - {WorkflowName: "CI", Status: "in_progress"}, - }, - } - stuck, reason := p.branchStuckReason(rs) - if !stuck { - t.Error("expected stuck=true for in_progress") - } - if reason != "prolonged_in_progress" { - t.Errorf("expected prolonged_in_progress, got %q", reason) - } -} - -func TestBranchNotStuckWhenSuccess(t *testing.T) { - cfg := &config.Config{} - cfg.Settings.StuckThresholdMinutes = 30 - p := &Poller{cfg: cfg, dispatcher: webhooks.New(nil)} - - rs := &state.RepoState{ - Runs: []githubclient.WorkflowRun{ - {WorkflowName: "CI", Status: "completed", Conclusion: "success"}, - }, - } - stuck, _ := p.branchStuckReason(rs) - if stuck { - t.Error("expected stuck=false for success") - } -} - -func TestPRStuckReasonConflict(t *testing.T) { - cfg := &config.Config{} - cfg.Settings.StuckThresholdMinutes = 30 - p := &Poller{cfg: cfg, dispatcher: webhooks.New(nil)} - - pr := &state.PRState{Mergeable: "dirty"} - stuck, reason := p.prStuckReason(pr) - if !stuck { - t.Error("expected stuck=true for dirty mergeable") - } - if reason != "conflict" { - t.Errorf("expected conflict, got %q", reason) - } -} - -func TestPRStuckReasonConflictingState(t *testing.T) { - cfg := &config.Config{} - cfg.Settings.StuckThresholdMinutes = 30 - p := &Poller{cfg: cfg, dispatcher: webhooks.New(nil)} - - pr := &state.PRState{Mergeable: "conflicting"} - stuck, reason := p.prStuckReason(pr) - if !stuck || reason != "conflict" { - t.Errorf("expected stuck=true/conflict, got stuck=%v reason=%q", stuck, reason) - } -} - -func TestPRNotStuckWhenClean(t *testing.T) { - cfg := &config.Config{} - cfg.Settings.StuckThresholdMinutes = 30 - p := &Poller{cfg: cfg, dispatcher: webhooks.New(nil)} - - pr := &state.PRState{ - Mergeable: "clean", - Runs: []githubclient.WorkflowRun{{WorkflowName: "CI", Status: "completed", Conclusion: "success"}}, - } - stuck, _ := p.prStuckReason(pr) - if stuck { - t.Error("expected stuck=false for clean/success") - } -} - -// stuckPoller builds a Poller with a controllable clock and a webhook endpoint, -// using a realistic 30-minute threshold rather than the zero threshold that a -// hand-built config can have but config.Load can never produce. -func stuckPoller(t *testing.T, received *[]webhooks.Event, now *time.Time) *Poller { - t.Helper() - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - var evt webhooks.Event - if err := json.NewDecoder(r.Body).Decode(&evt); err == nil { - *received = append(*received, evt) - } - w.WriteHeader(http.StatusOK) - })) - t.Cleanup(srv.Close) - - cfg := &config.Config{} - cfg.Settings.StuckThresholdMinutes = 30 - cfg.Webhooks = []config.Webhook{{URL: srv.URL}} - return &Poller{ - cfg: cfg, - dispatcher: webhooks.New(cfg.Webhooks), - stuck: make(map[string]*stuckEntry), - now: func() time.Time { return *now }, - } -} - -// dispatch mirrors what fetch does: evaluate, then POST whatever came back. -func (p *Poller) dispatchNow(repos []state.RepoState) { - for _, evt := range p.evaluateStuck(repos) { - p.dispatcher.Dispatch(evt) - } -} - -func failingRepo() []state.RepoState { - return []state.RepoState{{ - Owner: "o", - Name: "r", - Runs: []githubclient.WorkflowRun{{WorkflowName: "CI", Status: "completed", Conclusion: "failure"}}, - }} -} - -func TestStuckDoesNotFireBeforeThreshold(t *testing.T) { - var received []webhooks.Event - now := time.Date(2026, 8, 26, 9, 0, 0, 0, time.UTC) - p := stuckPoller(t, &received, &now) - - // Poll every 15s for 20 minutes. Nothing should fire before 30 minutes. - for i := 0; i < 80; i++ { - p.dispatchNow(failingRepo()) - now = now.Add(15 * time.Second) - } - - if len(received) != 0 { - t.Fatalf("expected no events before the threshold, got %d", len(received)) - } -} - -func TestStuckFiresOnceAfterThreshold(t *testing.T) { - var received []webhooks.Event - now := time.Date(2026, 8, 26, 9, 0, 0, 0, time.UTC) - p := stuckPoller(t, &received, &now) - - first := now - // Two hours of 30s polls across the 30-minute threshold. - for i := 0; i < 240; i++ { - p.dispatchNow(failingRepo()) - now = now.Add(30 * time.Second) - } - - if len(received) != 1 { - t.Fatalf("expected exactly 1 event, got %d", len(received)) - } - evt := received[0] - if evt.Event != "branch_stuck" { - t.Errorf("event = %q, want branch_stuck", evt.Event) - } - if evt.Reason != "prolonged_failure" { - t.Errorf("reason = %q, want prolonged_failure", evt.Reason) - } - if evt.Repo != "o/r" { - t.Errorf("repo = %q, want o/r", evt.Repo) - } - // StuckSince must be when the condition started, not when it alerted. - if !evt.StuckSince.Equal(first) { - t.Errorf("stuck_since = %v, want %v", evt.StuckSince, first) - } - if evt.Timestamp.Sub(evt.StuckSince) < 30*time.Minute { - t.Errorf("fired after %v, want at least the 30m threshold", evt.Timestamp.Sub(evt.StuckSince)) - } -} - -func TestStuckReArmsAfterRecovery(t *testing.T) { - var received []webhooks.Event - now := time.Date(2026, 8, 26, 9, 0, 0, 0, time.UTC) - p := stuckPoller(t, &received, &now) - - green := []state.RepoState{{ - Owner: "o", - Name: "r", - Runs: []githubclient.WorkflowRun{{WorkflowName: "CI", Status: "completed", Conclusion: "success"}}, - }} - - // First incident: stuck long enough to alert. - for i := 0; i < 5; i++ { - p.dispatchNow(failingRepo()) - now = now.Add(10 * time.Minute) - } - if len(received) != 1 { - t.Fatalf("first incident: expected 1 event, got %d", len(received)) - } - - // Recovers — the entry should be pruned. - p.dispatchNow(green) - if len(p.stuck) != 0 { - t.Fatalf("expected recovery to prune tracking, got %d entries", len(p.stuck)) - } - now = now.Add(10 * time.Minute) - - // Second incident: must alert again rather than staying silent. - for i := 0; i < 5; i++ { - p.dispatchNow(failingRepo()) - now = now.Add(10 * time.Minute) - } - if len(received) != 2 { - t.Fatalf("second incident: expected 2 events total, got %d", len(received)) - } -} - -func TestStuckReasonChangeRestartsTheClock(t *testing.T) { - var received []webhooks.Event - now := time.Date(2026, 8, 26, 9, 0, 0, 0, time.UTC) - p := stuckPoller(t, &received, &now) - - inProgress := []state.RepoState{{ - Owner: "o", - Name: "r", - Runs: []githubclient.WorkflowRun{{WorkflowName: "CI", Status: "in_progress"}}, - }} - - // 20 minutes in progress — under the threshold, nothing fires. - for i := 0; i < 2; i++ { - p.dispatchNow(inProgress) - now = now.Add(10 * time.Minute) - } - if len(received) != 0 { - t.Fatalf("expected nothing yet, got %d", len(received)) - } - - // It turns into a failure: a new condition, so the clock restarts and the - // 20 minutes already elapsed must not count toward the threshold. - p.dispatchNow(failingRepo()) - if len(received) != 0 { - t.Fatalf("reason change should restart the clock, got %d events", len(received)) - } - now = now.Add(31 * time.Minute) - p.dispatchNow(failingRepo()) - if len(received) != 1 { - t.Fatalf("expected 1 event after the new condition aged out, got %d", len(received)) - } - if received[0].Reason != "prolonged_failure" { - t.Errorf("reason = %q, want prolonged_failure", received[0].Reason) - } -} - -func TestStuckPRFiresWithPRInfo(t *testing.T) { - var received []webhooks.Event - now := time.Date(2026, 8, 26, 9, 0, 0, 0, time.UTC) - p := stuckPoller(t, &received, &now) - - repos := []state.RepoState{{ - Owner: "o", - Name: "r", - PRs: []state.PRState{{ - Number: 42, - Title: "Add a thing", - HTMLURL: "https://github.com/o/r/pull/42", - Mergeable: "dirty", - }}, - }} - - for i := 0; i < 5; i++ { - p.dispatchNow(repos) - now = now.Add(10 * time.Minute) - } - - if len(received) != 1 { - t.Fatalf("expected 1 event, got %d", len(received)) - } - evt := received[0] - if evt.Event != "pr_stuck" || evt.Reason != "conflict" { - t.Errorf("got %q/%q, want pr_stuck/conflict", evt.Event, evt.Reason) - } - if evt.PR == nil || evt.PR.Number != 42 { - t.Fatalf("expected PR 42 in the event, got %+v", evt.PR) - } -} - // recordingFetcher captures the queries it was asked to run, so a test can // assert what detail the poller requested. type recordingFetcher struct { @@ -590,28 +298,6 @@ func TestRateLimitBacksOffUntilReset(t *testing.T) { } } -// Stuck follows the Stoplight: every red conclusion is a failure and every -// yellow status is still going, not just failure/timed_out and in_progress. -func TestRunsStuckReasonFollowsStoplight(t *testing.T) { - cases := []struct { - run githubclient.WorkflowRun - stuck bool - reason string - }{ - {githubclient.WorkflowRun{Status: "completed", Conclusion: "action_required"}, true, "prolonged_failure"}, - {githubclient.WorkflowRun{Status: "completed", Conclusion: "startup_failure"}, true, "prolonged_failure"}, - {githubclient.WorkflowRun{Status: "queued"}, true, "prolonged_in_progress"}, - {githubclient.WorkflowRun{Status: "completed", Conclusion: "success"}, false, ""}, - {githubclient.WorkflowRun{Status: "completed", Conclusion: "cancelled"}, false, ""}, - } - for _, tc := range cases { - stuck, reason := runsStuckReason([]githubclient.WorkflowRun{tc.run}) - if stuck != tc.stuck || reason != tc.reason { - t.Errorf("%s/%s: got (%v, %q), want (%v, %q)", tc.run.Status, tc.run.Conclusion, stuck, reason, tc.stuck, tc.reason) - } - } -} - const loopConfig = ` [settings] poll_interval_seconds = 3600 @@ -685,3 +371,48 @@ func TestStopClosesTheChannel(t *testing.T) { for range ch { } } + +// A stuck branch reaches the webhook, and a slow endpoint does not hold the +// cycle up: delivery happens in the background. +func TestStuckAlertIsDeliveredWithoutBlockingTheCycle(t *testing.T) { + received := make(chan webhooks.Event, 1) + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var evt webhooks.Event + _ = json.NewDecoder(r.Body).Decode(&evt) + received <- evt + <-release // hang until the test is done + })) + defer srv.Close() + defer close(release) + + cfg := writeConfig(t, loopConfig+` +[[webhooks]] +url = "`+srv.URL+`" +`) + failing := []githubclient.WorkflowRun{{WorkflowName: "CI", Status: "completed", Conclusion: "failure"}} + p := New(cfg, stubFactory(failing, nil)) + start := time.Date(2026, 9, 28, 9, 0, 0, 0, time.UTC) + now := start + p.now = func() time.Time { return now } + + p.fetch(context.Background()) + now = start.Add(time.Duration(cfg.Settings.StuckThresholdMinutes)*time.Minute + time.Second) + + returned := make(chan struct{}) + go func() { p.fetch(context.Background()); close(returned) }() + select { + case <-returned: + case <-time.After(2 * time.Second): + t.Fatal("the cycle waited on the webhook endpoint") + } + + select { + case evt := <-received: + if evt.Event != "branch_stuck" || evt.Repo != "acme/one" { + t.Errorf("got %+v", evt) + } + case <-time.After(2 * time.Second): + t.Fatal("no webhook arrived") + } +} diff --git a/internal/ui/manage.go b/internal/ui/manage.go index 2e934a9..ea3fd5d 100644 --- a/internal/ui/manage.go +++ b/internal/ui/manage.go @@ -251,9 +251,9 @@ func (m Manage) listView() string { } line := fmt.Sprintf(" %s %-40s %s", toggle, name, branch) if i == m.cursor { - out += selectedStyle.Render("▶" + line) + "\n" + out += selectedStyle.Render("▶"+line) + "\n" } else { - out += style.Render(" " + line) + "\n" + out += style.Render(" "+line) + "\n" } } diff --git a/internal/webhooks/webhooks.go b/internal/webhooks/webhooks.go index 8e9c9e3..1580909 100644 --- a/internal/webhooks/webhooks.go +++ b/internal/webhooks/webhooks.go @@ -7,11 +7,11 @@ import ( "encoding/hex" "encoding/json" "fmt" - "log" "net/http" "time" "github.com/ericdahl-dev/git-green/internal/config" + "github.com/ericdahl-dev/git-green/internal/logx" ) // PRInfo carries PR metadata in an event payload. @@ -23,14 +23,14 @@ type PRInfo struct { // Event is the JSON payload POSTed to each webhook. type Event struct { - Event string `json:"event"` - Reason string `json:"reason"` - Repo string `json:"repo"` - PR *PRInfo `json:"pr,omitempty"` - Workflow string `json:"workflow"` - RunURL string `json:"run_url"` - StuckSince time.Time `json:"stuck_since"` - Timestamp time.Time `json:"timestamp"` + Event string `json:"event"` + Reason string `json:"reason"` + Repo string `json:"repo"` + PR *PRInfo `json:"pr,omitempty"` + Workflow string `json:"workflow"` + RunURL string `json:"run_url"` + StuckSince time.Time `json:"stuck_since"` + Timestamp time.Time `json:"timestamp"` } // Dispatcher sends webhook events to configured endpoints. @@ -47,8 +47,29 @@ func New(hooks []config.Webhook) *Dispatcher { } } -// Dispatch POSTs evt to all configured webhooks. Failures are logged but not -// returned — they must not interrupt the poll cycle. +// Send delivers evts to every configured webhook in the background and +// returns at once, so a slow or hanging endpoint never holds up polling. The +// returned channel closes when delivery has finished, for callers (tests) that +// need to wait. +func (d *Dispatcher) Send(evts []Event) <-chan struct{} { + done := make(chan struct{}) + if len(d.hooks) == 0 || len(evts) == 0 { + close(done) + return done + } + go func() { + defer close(done) + for _, evt := range evts { + d.Dispatch(evt) + } + }() + return done +} + +// Dispatch POSTs evt to all configured webhooks and waits for them. Failures +// are logged but not returned: an alert that cannot be delivered must not +// interrupt polling. Logging goes through logx, never plain stderr, which +// would scribble over the full-screen dashboard. func (d *Dispatcher) Dispatch(evt Event) { if len(d.hooks) == 0 { return @@ -56,13 +77,13 @@ func (d *Dispatcher) Dispatch(evt Event) { body, err := json.Marshal(evt) if err != nil { - log.Printf("webhooks: marshal error: %v", err) + logx.Debug("webhook marshal failed", "err", err) return } for _, wh := range d.hooks { if err := d.post(wh, body); err != nil { - log.Printf("webhooks: POST to %s failed: %v", wh.URL, err) + logx.Debug("webhook POST failed", "url", wh.URL, "err", err) } } }