Abort watchpr when a poll can no longer see the PR #12

2026-09-09T22:41:41+10:00

unkin-agent commented

2026-09-09 22:41:41 +10:00

A watched repo renamed, deleted, or made private mid-watch makes every poll 404 (Gitea hides invisible repos rather than 403ing), and the loop warned past it forever: the watcher looked healthy while reporting nothing. Transient failures had no cap either, so a wedged endpoint warned on every tick for the life of the process.

abort non-zero, naming the PR, when the PR lookup itself 404s
keep a 404 from any other call transient, so an ingress blip cannot kill a healthy watch
cap consecutive poll failures at 20 per PR (~19 minutes at the default interval)
reset the failure count on a successful poll
export IsPRGone so watchpr reports a vanished PR distinctly

A watched repo renamed, deleted, or made private mid-watch makes every poll 404 (Gitea hides invisible repos rather than 403ing), and the loop warned past it forever: the watcher looked healthy while reporting nothing. Transient failures had no cap either, so a wedged endpoint warned on every tick for the life of the process. - abort non-zero, naming the PR, when the PR lookup itself 404s - keep a 404 from any other call transient, so an ingress blip cannot kill a healthy watch - cap consecutive poll failures at 20 per PR (~19 minutes at the default interval) - reset the failure count on a successful poll - export IsPRGone so watchpr reports a vanished PR distinctly

unkin-agent added 1 commit 2026-09-09 22:41:42 +10:00

Abort watchpr when a poll can no longer see the PR

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

46dfe48adc

- treat a mid-run 404 on a tracked PR as terminal
- cap consecutive transient poll failures at 20 per PR
- reset the failure count on a successful poll
- export IsNotFound for callers to classify the abort

unkin-agent commented

2026-09-09 22:51:52 +10:00

Reviewed the diff and confirmed the build/tests pass (go build ./..., go test ./internal/agent/... -run TestWatch all green). PR body is within the convention cap. One substantive issue on the core design tension this PR is meant to resolve:

404 handling isn't scoped to "PR no longer visible" — it's scoped to "any of three calls 404'd," with zero retry tolerance

FetchState (internal/agent/watch.go:23-48) makes three calls: GetPR, CommitStatus, ListComments. Only CommitStatus's 404 is filtered (!IsNotFound(err), line 32) — deliberately, since a squash/rebase merge deletes the head branch and that's genuinely benign (confirmed TestWatchDetectsMergeWhenCommitGone still holds for the right reason). GetPR and ListComments both propagate any error, including 404, unfiltered.

Then in Watch's poll loop (watch.go:119-122), IsNotFound(err) on whatever FetchState returned is treated as immediately terminal — no retry, no counting toward MaxPollFailures, unlike every other error class (500/502/503/timeout/reset), which gets 20 consecutive tries. So:

A 404 from ListComments (issues/N/comments) is bucketed identically to "the repo/PR is gone," even though GetPR for the same number just succeeded moments earlier in the same poll. That's not "the tracked PR is invisible," that's one endpoint blipping.
Any transient/proxy 404 on either call — e.g. a k8s ingress-nginx default-backend 404 during a Gitea rolling restart, the "brief proxy/CDN 404" case — kills the watcher on the very first occurrence, with none of the 20-try tolerance this same PR just added for every other transient failure mode. That's the "dies on a transient blip" failure mode the fix is otherwise trying to avoid.

Suggest scoping the immediate-abort path to the GetPR 404 specifically (that's the actual identity check for "is this PR still visible"), and either dropping ListComments's error to a warning-and-continue like CommitStatus, or at minimum requiring 2+ consecutive 404s before declaring a PR gone rather than aborting on the first one.

Minor nit: the MaxPollFailures doc comment says the cap "rides out a 20-minute outage," but abort fires on the 20th consecutive failed tick — ~19 elapsed intervals (19 min at the default 60s) after the first failure. Not a functional bug, just imprecise wording.

Everything else checks out: the failure cap is genuinely per-ref (keyed by ref.String()), resets on success before the meaningful-change check, off-by-one is consistent with the stated "20 tries," and the two new abort tests would catch a revert of either condition while the two survival tests guard against over-correction. Diff is atomic — no auth-handling or --interval changes bundled in.

Reviewed the diff and confirmed the build/tests pass (`go build ./...`, `go test ./internal/agent/... -run TestWatch` all green). PR body is within the convention cap. One substantive issue on the core design tension this PR is meant to resolve: **404 handling isn't scoped to "PR no longer visible" — it's scoped to "any of three calls 404'd," with zero retry tolerance** `FetchState` (`internal/agent/watch.go:23-48`) makes three calls: `GetPR`, `CommitStatus`, `ListComments`. Only `CommitStatus`'s 404 is filtered (`!IsNotFound(err)`, line 32) — deliberately, since a squash/rebase merge deletes the head branch and that's genuinely benign (confirmed `TestWatchDetectsMergeWhenCommitGone` still holds for the right reason). `GetPR` and `ListComments` both propagate *any* error, including 404, unfiltered. Then in `Watch`'s poll loop (`watch.go:119-122`), `IsNotFound(err)` on whatever `FetchState` returned is treated as immediately terminal — no retry, no counting toward `MaxPollFailures`, unlike every other error class (500/502/503/timeout/reset), which gets 20 consecutive tries. So: - A 404 from `ListComments` (issues/N/comments) is bucketed identically to "the repo/PR is gone," even though `GetPR` for the same number just succeeded moments earlier in the same poll. That's not "the tracked PR is invisible," that's one endpoint blipping. - Any transient/proxy 404 on *either* call — e.g. a k8s ingress-nginx default-backend 404 during a Gitea rolling restart, the "brief proxy/CDN 404" case — kills the watcher on the very first occurrence, with none of the 20-try tolerance this same PR just added for every other transient failure mode. That's the "dies on a transient blip" failure mode the fix is otherwise trying to avoid. Suggest scoping the immediate-abort path to the `GetPR` 404 specifically (that's the actual identity check for "is this PR still visible"), and either dropping `ListComments`'s error to a warning-and-continue like `CommitStatus`, or at minimum requiring 2+ consecutive 404s before declaring a PR gone rather than aborting on the first one. Minor nit: the `MaxPollFailures` doc comment says the cap "rides out a 20-minute outage," but abort fires on the 20th consecutive failed tick — ~19 elapsed intervals (19 min at the default 60s) after the first failure. Not a functional bug, just imprecise wording. Everything else checks out: the failure cap is genuinely per-ref (keyed by `ref.String()`), resets on success before the meaningful-change check, off-by-one is consistent with the stated "20 tries," and the two new abort tests would catch a revert of either condition while the two survival tests guard against over-correction. Diff is atomic — no auth-handling or `--interval` changes bundled in.

unkin-agent added 1 commit 2026-09-09 22:56:02 +10:00

Scope watchpr's terminal 404 to the PR lookup

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

d04c5aa58d

Only a 404 from GetPR means the PR is gone. A 404 from any other call
can be a proxy or ingress blip, so it now warns and counts against the
consecutive-failure cap instead of killing the watch on first sight.

benvin merged commit 5c0eb1e899 into main

2026-09-09 23:13:17 +10:00

benvin deleted branch benvin/watchpr-terminal-errors

2026-09-09 23:13:17 +10:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/agent-tools#12