diff --git a/AGENTS.md b/AGENTS.md index b5772f1..dd5ae51 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -178,6 +178,9 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`. ## Gotchas - `watchpr` exits 0 with no output changes on `--once` (just prints state). +- Background commands are killed at a 2h cap, indistinguishable from a crash. + Orchestrators run `watchpr --max-wait 110m ...` and restart it on exit code 3 + (no change within max-wait); any other non-zero exit is a real failure. - Gitea tokens expire in ~1h, shorter than a watch: the client re-mints once when the credential it sent was rejected and replays the request. If the fresh token is rejected too, `watchpr` exits non-zero rather than polling blind. diff --git a/README.md b/README.md index efc19bb..549bc3a 100644 --- a/README.md +++ b/README.md @@ -131,11 +131,20 @@ watchpr --interval 30 unkin/argocd-apps#42 # One-shot: print current state and exit 0 (great for scripts) watchpr --once unkin/argocd-apps#42 watchpr --once --json unkin/argocd-apps#42 + +# Orchestrators: run inside a background command (2h hard cap) and restart on exit 3 +watchpr --max-wait 110m unkin/argocd-apps#42 ``` On a meaningful change `watchpr` prints the reason and the PR's current state, then exits 0. Use `--json` for machine-readable output. +`--max-wait` (same forms as `--interval`; default `0` = unlimited) bounds a +watch: when it elapses with no change, `watchpr` prints `timeout: no change +within ` and each PR's current state (`{"timeout":true,...}` under `--json`) +and exits **3**, so a timed-out watch is distinguishable from a crash and is +safe to restart. + ### Exit behaviour A watcher that sees nothing must not look healthy, so every terminal failure diff --git a/cmd/watchpr/main.go b/cmd/watchpr/main.go index a5a155a..39637a9 100644 --- a/cmd/watchpr/main.go +++ b/cmd/watchpr/main.go @@ -19,10 +19,12 @@ // watchpr --once --json owner/repo#12 // watchpr --interval 30s owner/repo#12 // watchpr --interval 30 owner/repo#12 +// watchpr --max-wait 110m owner/repo#12 # exits 3 if nothing changed package main import ( "encoding/json" + "errors" "fmt" "io" "os" @@ -36,19 +38,34 @@ import ( var version = "dev" +// exitTimedOut is the exit status when --max-wait elapses with no change, so a +// caller can tell a deliberate timeout from a crash and restart the watch. +const exitTimedOut = 3 + +var errTimedOut = errors.New("max-wait elapsed with no change") + func main() { // cobra prints the error itself (SilenceErrors stays off); we only need to // turn any command error into a non-zero exit. - if err := newRootCmd().Execute(); err != nil { - os.Exit(1) + os.Exit(exitCode(newRootCmd().Execute())) +} + +// exitCode maps a command result to the process exit status. +func exitCode(err error) int { + switch { + case err == nil: + return 0 + case errors.Is(err, errTimedOut): + return exitTimedOut } + return 1 } // newRootCmd builds the watchpr command tree. It is separated from main so // tests can invoke Execute and assert the exit behaviour without spawning a // process. func newRootCmd() *cobra.Command { - var intervalFlag string + var intervalFlag, maxWaitFlag string var once, jsonMode bool root := &cobra.Command{ @@ -57,7 +74,9 @@ func newRootCmd() *cobra.Command { Long: "watchpr polls each PR every --interval and exits (reporting what changed)\n" + "when a PR merges/closes, gets a new non-agent comment, its CI fails, or it\n" + "loses mergeability after the baseline. Conditions already true at the\n" + - "baseline are printed, not alerted on. Refs take owner/repo#N or owner/repo:N.", + "baseline are printed, not alerted on. Refs take owner/repo#N or owner/repo:N.\n\n" + + "Exit status: 0 on a change (or --once), 3 when --max-wait elapses with no\n" + + "change (current states are printed), 1 on any error.", Version: version, Args: cobra.ArbitraryArgs, SilenceUsage: true, @@ -69,6 +88,13 @@ func newRootCmd() *cobra.Command { if err != nil { return err } + maxWait, err := parseMaxWait(maxWaitFlag) + if err != nil { + return err + } + if once && maxWait > 0 { + return fmt.Errorf("--max-wait has no effect with --once") + } refs := make([]agent.PRRef, 0, len(args)) for _, a := range args { ref, err := agent.ParsePRRef(a) @@ -81,13 +107,18 @@ func newRootCmd() *cobra.Command { if once { return runOnce(c, refs, jsonMode) } - return runWatch(c, refs, interval, jsonMode) + err = runWatch(c, refs, interval, maxWait, jsonMode) + if errors.Is(err, errTimedOut) { + cmd.SilenceErrors = true + } + return err }, } root.SetVersionTemplate("{{.Version}}\n") f := root.Flags() f.StringVar(&intervalFlag, "interval", "60s", "Polling interval: a duration (30s, 2m, 1h30m) or a bare number of seconds") + f.StringVar(&maxWaitFlag, "max-wait", "0", "Give up after this long with no change and exit 3 (same forms as --interval; 0 = unlimited)") f.BoolVar(&once, "once", false, "Check once, print current state, and exit") f.BoolVar(&jsonMode, "json", false, "Emit JSON") @@ -154,11 +185,17 @@ func runOnce(c *agent.GiteaClient, refs []agent.PRRef, jsonMode bool) error { // runWatch establishes a baseline then polls until a tracked PR changes // meaningfully, at which point it reports the change and returns. -func runWatch(c *agent.GiteaClient, refs []agent.PRRef, interval time.Duration, jsonMode bool) error { +func runWatch(c *agent.GiteaClient, refs []agent.PRRef, interval, maxWait time.Duration, jsonMode bool) error { login := agent.AgentLogin() ticker := time.NewTicker(interval) defer ticker.Stop() + var deadline <-chan time.Time + if maxWait > 0 { + timer := time.NewTimer(maxWait) + defer timer.Stop() + deadline = timer.C + } onBaseline := func(states []agent.PRState) { emitBaselines(os.Stderr, states, interval, jsonMode) @@ -167,14 +204,65 @@ func runWatch(c *agent.GiteaClient, refs []agent.PRRef, interval time.Duration, warn(os.Stderr, jsonMode, "polling %s: %v", ref.String(), err) } - res, err := agent.Watch(c, refs, login, ticker.C, onBaseline, onError) + res, err := agent.Watch(c, refs, login, untilDeadline(ticker.C, deadline), onBaseline, onError) if err != nil { return describeFailure(err) } + if res.TimedOut { + reportTimeout(os.Stdout, maxWait, res.States, jsonMode) + return errTimedOut + } report(res.Ref.String(), res.Reason, res.State, jsonMode) return nil } +// parseMaxWait reads --max-wait: 0 means unlimited, anything else parses like +// --interval. +func parseMaxWait(v string) (time.Duration, error) { + if d, err := time.ParseDuration(strings.TrimSpace(v)); err == nil && d == 0 { + return 0, nil + } + return agent.ParseDurationFlag("max-wait", v) +} + +// untilDeadline forwards ticks until deadline fires, then closes, which ends +// Watch without waiting out the rest of an interval. A nil deadline never fires. +func untilDeadline(ticks, deadline <-chan time.Time) <-chan time.Time { + out := make(chan time.Time) + go func() { + defer close(out) + for { + select { + case <-deadline: + return + case t := <-ticks: + select { + case out <- t: + case <-deadline: + return + } + } + } + }() + return out +} + +// reportTimeout emits each PR's current state when --max-wait elapses. +func reportTimeout(w io.Writer, maxWait time.Duration, states []agent.PRState, jsonMode bool) { + if jsonMode { + _ = json.NewEncoder(w).Encode(struct { + Timeout bool `json:"timeout"` + MaxWait string `json:"max_wait"` + States []agent.PRState `json:"states"` + }{true, maxWait.String(), states}) + return + } + _, _ = fmt.Fprintf(w, "timeout: no change within %s\n", maxWait) + for _, st := range states { + _, _ = fmt.Fprintln(w, stateLine(st)) + } +} + // baselineRecord is the --json form of baselineLine. Both go to stderr, leaving // the stdout contract a single result record: a caller automating watchpr is // precisely the one who needs to be told the watch started against a PR that is diff --git a/cmd/watchpr/main_test.go b/cmd/watchpr/main_test.go index 62340f1..fff71d0 100644 --- a/cmd/watchpr/main_test.go +++ b/cmd/watchpr/main_test.go @@ -311,3 +311,96 @@ func TestBaselineNamesAnUnknownMergeability(t *testing.T) { t.Errorf("baselineLine = %q, want the suppression note", got) } } + +func TestParseMaxWait(t *testing.T) { + for in, want := range map[string]time.Duration{"0": 0, "0s": 0, "30s": 30 * time.Second, "1h55m": 115 * time.Minute, "90": 90 * time.Second} { + got, err := parseMaxWait(in) + if err != nil || got != want { + t.Errorf("parseMaxWait(%q) = %v, %v; want %v", in, got, err, want) + } + } + for _, in := range []string{"soon", "-1m"} { + if _, err := parseMaxWait(in); err == nil || !strings.Contains(err.Error(), "--max-wait") { + t.Errorf("parseMaxWait(%q) err = %v, want a --max-wait error", in, err) + } + } +} + +func TestExecuteBadMaxWaitErrors(t *testing.T) { + cmd := newRootCmd() + cmd.SetArgs([]string{"--max-wait", "soon", "unkin/repo#1"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + if err := cmd.Execute(); err == nil || errors.Is(err, errTimedOut) { + t.Fatalf("Execute() = %v, want a parse error", err) + } +} + +func TestExecuteOnceWithMaxWaitErrors(t *testing.T) { + cmd := newRootCmd() + cmd.SetArgs([]string{"--once", "--max-wait", "5m", "unkin/repo#1"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "--once") { + t.Fatalf("Execute() = %v, want a --once/--max-wait conflict error", err) + } + if got := exitCode(err); got != 1 { + t.Fatalf("exitCode = %d, want 1", got) + } +} + +func TestExitCode(t *testing.T) { + for _, tc := range []struct { + err error + want int + }{ + {nil, 0}, + {errTimedOut, exitTimedOut}, + {fmt.Errorf("wrapped: %w", errTimedOut), exitTimedOut}, + {errors.New("boom"), 1}, + } { + if got := exitCode(tc.err); got != tc.want { + t.Errorf("exitCode(%v) = %d, want %d", tc.err, got, tc.want) + } + } +} + +// Ticks pass through until the deadline fires; then the channel closes without +// waiting for another tick. +func TestUntilDeadline(t *testing.T) { + ticks := make(chan time.Time) + deadline := make(chan time.Time) + out := untilDeadline(ticks, deadline) + + now := time.Now() + ticks <- now + if got := <-out; !got.Equal(now) { + t.Fatalf("forwarded %v, want %v", got, now) + } + close(deadline) + if _, ok := <-out; ok { + t.Fatal("channel still open after the deadline") + } +} + +func TestReportTimeout(t *testing.T) { + st := agent.PRState{Ref: agent.PRRef{Owner: "unkin", Repo: "repo", Number: 3}, State: "open", Mergeable: agent.MergeYes, CIStatus: "pending"} + + var buf bytes.Buffer + reportTimeout(&buf, 110*time.Minute, []agent.PRState{st}, false) + lines := strings.Split(strings.TrimSpace(buf.String()), "\n") + if len(lines) != 2 || lines[0] != "timeout: no change within 1h50m0s" || lines[1] != stateLine(st) { + t.Errorf("text output = %q", buf.String()) + } + + buf.Reset() + reportTimeout(&buf, time.Minute, []agent.PRState{st}, true) + var rec struct { + Timeout bool `json:"timeout"` + States []agent.PRState `json:"states"` + } + if err := json.Unmarshal(buf.Bytes(), &rec); err != nil || !rec.Timeout || len(rec.States) != 1 { + t.Errorf("json output = %q (err %v)", buf.String(), err) + } +} diff --git a/internal/agent/watch.go b/internal/agent/watch.go index ac6be24..42c5d3b 100644 --- a/internal/agent/watch.go +++ b/internal/agent/watch.go @@ -124,11 +124,14 @@ func (c *GiteaClient) FetchState(ref PRRef, agentLogin string) (PRState, error) return FetchState(c, ref, agentLogin) } -// WatchResult is the change that ended a watch. +// WatchResult is the change that ended a watch. When the tick channel closes +// with no change, TimedOut is set and States holds each PR's last seen state. type WatchResult struct { - Ref PRRef - Reason string - State PRState + Ref PRRef + Reason string + State PRState + TimedOut bool + States []PRState } // terminalState reports whether a PR has reached a final state from which no @@ -276,6 +279,7 @@ const MaxPollFailures = 20 // look healthy. // onBaseline, if set, receives every captured baseline once, before the first // tick, so a caller can show what state the watch started from. +// A closed ticks channel ends the watch with a TimedOut result. func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Time, onBaseline func([]PRState), onError func(PRRef, error)) (WatchResult, error) { watches := make(map[string]*prWatch, len(refs)) baselines := make([]PRState, 0, len(refs)) @@ -293,11 +297,12 @@ func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Ti if onBaseline != nil { onBaseline(baselines) } + latest := append([]PRState(nil), baselines...) fails := make(map[string]int, len(refs)) // The tick carries the time it fired, which is the clock the conflict // window is measured on. for now := range ticks { - for _, ref := range refs { + for i, ref := range refs { key := ref.String() cur, err := f.FetchState(ref, agentLogin) if err != nil { @@ -315,12 +320,13 @@ func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Ti continue } fails[key] = 0 + latest[i] = cur if changed, reason := watches[key].observe(cur, now); changed { return WatchResult{Ref: ref, Reason: reason, State: cur}, nil } } } - return WatchResult{}, nil + return WatchResult{TimedOut: true, States: latest}, nil } // countNonAgentComments counts comments authored by anyone other than agentLogin. diff --git a/internal/agent/watch_test.go b/internal/agent/watch_test.go index b153fbc..dba33ac 100644 --- a/internal/agent/watch_test.go +++ b/internal/agent/watch_test.go @@ -1389,3 +1389,23 @@ func TestWatchConflictWindowCountsFromTheZeroTime(t *testing.T) { t.Fatalf("reason = %q, want the mergeability loss; a run starting at the zero time still counts", res.Reason) } } + +// Closed ticks with no change end the watch as a timeout carrying the latest +// state seen, not the baseline. +func TestWatchTimesOutWithLatestStates(t *testing.T) { + open := base() + pushed := base() + pushed.HeadSHA = "def456" + f := &fakeFetcher{states: []PRState{open, pushed}} + + res, err := Watch(f, []PRRef{open.Ref}, "unkin-agent", drainableTicks(2), nil, nil) + if err != nil { + t.Fatalf("Watch: %v", err) + } + if !res.TimedOut { + t.Fatalf("TimedOut = false, want true") + } + if len(res.States) != 1 || res.States[0].HeadSHA != "def456" { + t.Errorf("States = %+v, want the latest polled state", res.States) + } +}