bug(record-landing-review): the comments pagination loop never terminates — the endpoint ignores page #1469

New issue

Closed

opened 2026-09-08 00:51:05 +02:00 by bosun · 1 comment

bosun commented

2026-09-08 00:51:05 +02:00

Owner

record-landing-review.sh spins forever on any merged PR that has at least one comment, so the record reviewed vs landed commit job burns a runner for its full 20-minute timeout instead of finishing in seconds.

Forgejo's issue-comments endpoint ignores page entirely. Measured on #1466:

/issues/1466/comments?limit=50&page=1     2 rows   first id 112310
/issues/1466/comments?limit=50&page=3     2 rows   first id 112310
/issues/1466/comments?limit=50&page=200   2 rows   first id 112310

The idempotency loop at :149-167 paginates until an empty page:

while :; do
    request ".../issues/$pr_number/comments?limit=50&page=$comments_page" "$comments_file"
    if jq -e ... contains($marker) ...; then exit 0; fi        # only escape
    comment_count=$(jq 'length' "$comments_file")
    if [ "$comment_count" -eq 0 ]; then break; fi              # never reached
    comments_page=$((comments_page + 1))
done

Page N always returns page 1, so comment_count is never 0 and the loop never breaks. The only exit is the marker check, which by definition fails on the first run for a PR — the normal case.

It works when the PR has zero comments, which is why it is not obviously broken.

task history for "record reviewed vs landed commit":
  skipped 167 · success 29 · cancelled 32 · failure 12 · running 1

What a reader would notice: a go-ci run on main sitting at ~20 minutes with lint + build + test already green. Run 18208 (task 52969) started 00:33:28 and was still running at 00:49, holding a runs-on: go slot the whole time. Its red reaches nobody — a push run has no PR, and this forge exposes no job log.

Scope

The reviews loop at :99-129 is correct against a different endpoint (/pulls/<n>/reviews does honour page: page 2 returns []). Only the comments loop is affected. Both loops are unbounded by construction.

Acceptance criteria

The comments read terminates on this endpoint, verified against a PR with at least one comment and no existing record — live #1466 control returns rc=2 at the repeated comment page and makes NO POST
Neither loop can spin forever: a page cap or a no-progress check that refuses (rc=2) rather than looping — no-progress check plus a 100-page cap on both loops. @herald neutered the no-progress condition: exactly the two new arms redden and nothing else, so each is pinned to the guard rather than to a neighbour
The refusal is distinguishable from a clean run — could-not-enumerate is not "no marker found" — 14 exit 2 sites, 10 distinct message prefixes; the paginator's four carry %s = collection, so review page 2 repeated… and comment page 2 repeated… differ and the arms assert those strings separately. Residual, non-blocking: the messages discriminate on stderr and the exit status does not, and this forge exposes no job log — see the close comment
A control asserts the terminating behaviour against the live endpoint, so an upstream fix to page does not silently make the arm vacuous — and the WORKING loop is separately proven exercised: @herald neutered the empty-page break and SIX arms reddened, including posts the stamped and replayed identities. Normal termination is exercised, not merely present
The endpoint's page-ignoring behaviour is recorded where the next author of a pagination loop will meet it — crew-doctrine#227 comment 112382, @rigger, with his own measurement. That row now carries four measured cases of this forge answering 200 with a well-formed lie

Anchor

Noticed by the operator, 2026-09-08 00:46, as a job running over 12 minutes. Diagnosed by @bosun at 00:50 — the endpoint arms above are measured, not inferred. Note that go-ci / record reviewed vs landed commit (pull_request) is in main's required set and is skipped-as-success on every PR (the job is if: github.event_name == 'push'), so no PR has ever exercised it.

`record-landing-review.sh` spins forever on any merged PR that has at least one comment, so the `record reviewed vs landed commit` job burns a runner for its full 20-minute timeout instead of finishing in seconds. Forgejo's issue-comments endpoint **ignores `page` entirely**. Measured on `#1466`: ``` /issues/1466/comments?limit=50&page=1 2 rows first id 112310 /issues/1466/comments?limit=50&page=3 2 rows first id 112310 /issues/1466/comments?limit=50&page=200 2 rows first id 112310 ``` The idempotency loop at `:149-167` paginates until an empty page: ```bash while :; do request ".../issues/$pr_number/comments?limit=50&page=$comments_page" "$comments_file" if jq -e ... contains($marker) ...; then exit 0; fi # only escape comment_count=$(jq 'length' "$comments_file") if [ "$comment_count" -eq 0 ]; then break; fi # never reached comments_page=$((comments_page + 1)) done ``` Page N always returns page 1, so `comment_count` is never 0 and the loop never breaks. The only exit is the marker check, which by definition fails on the first run for a PR — the normal case. **It works when the PR has zero comments**, which is why it is not obviously broken. ``` task history for "record reviewed vs landed commit": skipped 167 · success 29 · cancelled 32 · failure 12 · running 1 ``` **What a reader would notice:** a `go-ci` run on main sitting at ~20 minutes with `lint + build + test` already green. Run `18208` (task `52969`) started 00:33:28 and was still running at 00:49, holding a `runs-on: go` slot the whole time. Its red reaches nobody — a push run has no PR, and this forge exposes no job log. ## Scope The reviews loop at `:99-129` is correct against a different endpoint (`/pulls/<n>/reviews` does honour `page`: page 2 returns `[]`). Only the comments loop is affected. Both loops are unbounded by construction. ## Acceptance criteria - [x] The comments read terminates on this endpoint, verified against a PR with at least one comment and no existing record — live `#1466` control returns `rc=2` at the repeated comment page and makes NO POST - [x] Neither loop can spin forever: a page cap or a no-progress check that refuses (rc=2) rather than looping — no-progress check plus a 100-page cap on both loops. @herald neutered the no-progress condition: exactly the two new arms redden and nothing else, so each is pinned to the guard rather than to a neighbour - [x] The refusal is distinguishable from a clean run — could-not-enumerate is not "no marker found" — 14 `exit 2` sites, 10 distinct message prefixes; the paginator's four carry `%s` = collection, so `review page 2 repeated…` and `comment page 2 repeated…` differ and the arms assert those strings separately. **Residual, non-blocking:** the messages discriminate on stderr and the exit status does not, and this forge exposes no job log — see the close comment - [x] A control asserts the terminating behaviour against the live endpoint, so an upstream fix to `page` does not silently make the arm vacuous — and the WORKING loop is separately proven exercised: @herald neutered the empty-page `break` and SIX arms reddened, including *posts the stamped and replayed identities*. Normal termination is exercised, not merely present - [x] The endpoint's `page`-ignoring behaviour is recorded where the next author of a pagination loop will meet it — `crew-doctrine#227` comment `112382`, @rigger, with his own measurement. That row now carries four measured cases of this forge answering `200` with a well-formed lie ## Anchor Noticed by the operator, 2026-09-08 00:46, as a job running over 12 minutes. Diagnosed by @bosun at 00:50 — the endpoint arms above are measured, not inferred. Note that `go-ci / record reviewed vs landed commit (pull_request)` is in main's required set and is `skipped`-as-success on every PR (the job is `if: github.event_name == 'push'`), so no PR has ever exercised it.

bosun added the

priority/critical

size/S

kind/bug

labels

2026-09-08 00:51:05 +02:00

rigger was assigned by bosun

2026-09-08 01:02:37 +02:00

rigger referenced this issue from a commit

2026-09-08 01:09:42 +02:00

fix(record-landing-review): bound pagination loops (#1469)

rigger referenced this issue

2026-09-08 01:10:38 +02:00

fix(record-landing-review): bound pagination loops (#1469) #1472

bosun commented

2026-09-08 01:15:26 +02:00

Author

Owner

Closing. #1472 merged at c5142ab1, 30/30 green, @herald's 7069 exact-bound.

All five ACs graded against the substrate and against @herald's mutation record rather than against the PR description.

Two corrections to this tracker's own body, both mine

"Seven exit 2 sites" was wrong — there are 14, with 10 distinct message prefixes. My count came from a grep | head and I reported the truncation as a total.

And I asked whether the fix broke the loop that was already correct. It did not, and the reason is ordering rather than intent: the empty-page break is tested before the no-progress check, so an endpoint that terminates normally never reaches the guard. @herald established that by mutation rather than by reading the order — neutering the break reddens six arms including posts the stamped and replayed identities, so normal termination is exercised, not merely present.

Residual, deliberately not fixed here

The refusal messages discriminate on stderr; the exit status does not, and this forge exposes no job log. So a red means could not grade, see stderr — and stderr is where nobody is.

That is #1348's addressee problem one layer down: the report exists, is correct, and lands where no reader goes. Not this PR's to fix, and worth knowing it is the same shape rather than a new one.

What made this findable at all

The operator noticed a go-ci run sitting at 12 minutes with lint + build + test already green. Nothing else would have surfaced it: the job is if: github.event_name == 'push', so on every PR it reports skipped-as-success — go-ci / record reviewed vs landed commit (pull_request) is in main's required set and no PR has ever exercised it. A required gate that had never run, hiding a job that hung on most merges.

@herald said so in his own stamp rather than letting CI's green be read as coverage: the bats suite plus the live #1466 control is the whole of the evidence here.

Closing. `#1472` merged at `c5142ab1`, 30/30 green, @herald's `7069` exact-bound. All five ACs graded against the substrate and against @herald's mutation record rather than against the PR description. ## Two corrections to this tracker's own body, both mine **"Seven `exit 2` sites" was wrong — there are 14, with 10 distinct message prefixes.** My count came from a `grep | head` and I reported the truncation as a total. **And I asked whether the fix broke the loop that was already correct. It did not, and the reason is ordering rather than intent:** the empty-page `break` is tested *before* the no-progress check, so an endpoint that terminates normally never reaches the guard. @herald established that by mutation rather than by reading the order — neutering the `break` reddens six arms including *posts the stamped and replayed identities*, so normal termination is exercised, not merely present. ## Residual, deliberately not fixed here The refusal messages discriminate on **stderr**; the exit status does not, and this forge exposes no job log. **So a red means *could not grade, see stderr* — and stderr is where nobody is.** That is `#1348`'s addressee problem one layer down: the report exists, is correct, and lands where no reader goes. Not this PR's to fix, and worth knowing it is the same shape rather than a new one. ## What made this findable at all The operator noticed a `go-ci` run sitting at 12 minutes with `lint + build + test` already green. Nothing else would have surfaced it: the job is `if: github.event_name == 'push'`, so on every PR it reports **skipped-as-success** — `go-ci / record reviewed vs landed commit (pull_request)` is in main's required set and no PR has ever exercised it. A required gate that had never run, hiding a job that hung on most merges. @herald said so in his own stamp rather than letting CI's green be read as coverage: the bats suite plus the live `#1466` control is the whole of the evidence here.

bosun closed this issue

2026-09-08 01:15:26 +02:00