[nissa-seru]

The polling fallback could lose rows without saying so. This reads each …Since list by updated_at instead of by page offset, keeps a cursor in whole seconds for each repository's three lists, and lets a read stopped by the request bound carry on from where it stopped.

The problem

Each of these was run on main (6b5f8e2), with the real adapter against the test file's fake GitHub server, and the poller through pollOnce:

• A short page ended the read. With 149 rows updated since the cursor and a first page of 99 that carried Link: rel="next", listIssuesSince returned 99 and never asked for page 2.
• The page cap moved the cursor past rows it never read. With 350 rows, a pass delivered 300, logged "the cursor will pick the rest up next tick", and moved the cursor to the start of the pass. The next pass asked from there and delivered nothing, so 50 rows were never asked for again.
• A row updated mid-read could hide another. With 150 rows, and Contributor mode: follow your own PRs and threads in repos where the App isn't installed (user token + /notifications) #5 updated between page 1 and page 2, Contributor mode: follow your own PRs and threads in repos where the App isn't installed (user token + /notifications) #5 moved to the end of the list, so #101 slid onto page 1, which had already been read. After two passes #101 had never been served. Following rel="next" alone doesn't fix this one.

Two more came up in review, both older than this change:

• A cursor part way through a second. The cursor was the pass's start, with milliseconds, and GitHub's since part way through a second leaves out every row stamped in that second. Asked with gh api on cli/cli's issue comments, since = a comment's own second returned it, and since = that second plus 1 ms, 100 ms or 999 ms left out all six comments tried. So a row changed later in the second a pass started in, after its read, wasn't read again unless it changed again.
• A refused first pass. With no stored cursor, a pass reads from one interval back. If a delivery was refused, nothing was stored, so the retry's window moved on with the clock and could leave the refused row behind.

getCheckRuns had the same short-page stop. It failed safe, since a sha whose runs weren't all read is never reported as success. But the poller waits on an undecided head without spending its attempts, so such a head would be asked about again on every pass.

What changes

• pollSince replaces pollPages. Each request after the first asks since = the last row's updated_at, page 1, so a row updated during the read can only move into what is still to be read. GitHub's since includes its own second, as above, so the rows of that second come back. They're kept once, by id, as the later copy. Only Link: rel="next" says more follow.
• One second's rows are paged by offset. A page that is all one second goes on to that second's next page. That holds while GitHub keeps rows that share a second in the same order from page to page, which I haven't checked.
• A stop never cuts inside a second. After POLL_MAX_PAGES (3) requests, the read stops before its next request that doesn't page through one second. So it goes past the bound only to finish a second it is part way through, keeping the rest of the page that finishes it. It leaves out the stop second's rows and returns resumeAt, the second it stopped in, and the next read asks from there. So no row is skipped, and none of the rows this read returned comes back unchanged.
• The list methods return PollRead<T>, which is { rows, resumeAt? }. POLL_MAX_ENTRIES goes: a read is bounded by its requests, and goes past them only to finish a second.
• One cursor per repository and list, in whole seconds. These are kept in the state file as pollCursors, an additive field, so a version-1 file without it still loads. A list that finished never re-reads because another list stopped, so the 2,000-key dedup ring no longer has to cover other lists' re-reads. A list read to the end moves to the second its pass started in, so that second is read again next time, and the ring drops what was already delivered.
  • lastPollAt is now the earliest of the pass's start and every point where a read stopped, so every list of the repositories polled has delivered what changed before it. A list with no cursor of its own starts there: one of a repository newly in GITHUB_REPOS, or every list of a state file from before this change. A release from before this change reads only lastPollAt, so after a rollback it reads on from the earliest unread point.
  • Cursors are written only when the whole pass delivered, as the single cursor was. A repository no longer in GITHUB_REPOS drops out of them, so if it's added back it starts from lastPollAt like any new one. A first pass stores the window it reads, as lastPollAt, before it reads, so a pass that's refused or fails partway is retried from the same start.
  • Stored positions are read as whole seconds, so a state file from main, whose cursor carries milliseconds, starts at its second. The loader drops a cursor whose since isn't an instant, a resumeAt that isn't one, and a lastPollAt that isn't one, with a warning for the last, so a damaged value never reaches GitHub.
• "Opened" is said once per row. Amendment 2a calls a row new when its created_at is at or after its list's window start. A stopped issues read keeps that start, so an issue opened in the window but changed past the stop still arrives as issue.opened rather than issue.edited.
  • A row can be read again inside its window: after a stop, after a refused delivery, or in the second a pass started in. So the state file now remembers, in pollNew, each row announced as new, from the moment its event is accepted, until its created_at falls before its list's window. A remembered row is judged as not new.
  • Head markers can't stand in for this, since a stopped read means about 300 issue rows in a pass, past POLL_HEAD_MEMORY's 200 markers.
• getCheckRuns reads on while total_count says more runs follow, whatever a page's length. A response without a total_count, which main read only one page of, now reads on past a full page, and is undecided if the page bound stops it. An empty page ends the read: the head isn't called a success, and stays undecided unless a run that was read failed. The warning says the count wasn't reached, not the bound.

What each part is for, and what was cut

• Reading by updated_at is what fixes the third loss: following rel="next" by offset still skips #101. rel="next" fixes the first.
• Paging within one second keeps a page that is all one second from asking for the same page again. Not cutting inside a second keeps a run longer than the bound from stopping the list at the same place every pass. A page with no stamp to read on from is paged the same way, rather than asking since from nothing.
• resumeAt fixes the second loss. The alternative, one cursor moved to where the earliest list stopped, re-reads every other list, and many repositories would overflow the dedup ring.
• Whole seconds fix the cursor that started part way through a second. The cost is one second's rows read again each pass.
• lastPollAt as the earliest stop keeps its meaning on main, which makes a rollback and a dropped cursor safe. The cost: while a list is catching up, a newly added repository starts from that earlier point.
• The first pass's window is fixed with one line, which stores the window before the pass reads.
• Reading stored positions as whole seconds, and the loader's instant checks, cover state files this change didn't write: one from main, or a damaged one.
• pollNew is the smallest exact record I found for "opened once". Without it, an issue opened in the window and read again there is announced as opened twice. Moving the window's start to the stop instead would make issues opened before the stop, but changed past it, arrive as edited. Its prune, when a pass delivers, keeps it to the rows created inside each window. Without that, it would only grow.
• getCheckRuns's total_count stop replaces its short-page stop. It was already its other stop. The full-page rule without a total_count keeps such a response from being decided on its first page, and undecided at the bound keeps a read that ran out of pages from being called a success. The empty-page stop keeps a total_count that counts runs no longer there from spending the whole page bound on every pass, and its warning says which stop it was.
• Cut while building: POLL_MAX_ENTRIES, which the request bound makes redundant; reordering a row read again to its later place, since the poller sorts by updated_at anyway; a stop at an empty page in the …Since lists, since there only rel="next" decides the end; and a record of opened ids kept only while a stopped read is caught up, which a refused pass lost and a finished read cleared too early.

Tests

• Full check at 08dc191 (this branch is on main, so the merge is the head; npm ci, npm run typecheck, npm test on Node 24.22.0): typecheck clean, 606 tests, 586 pass, 2 fail, 18 skipped. The 28 more are this change's 29 new tests less the one removed. main at 6b5f8e2, same commands and host: 578 tests, 558 pass, 2 fail, 18 skipped. The two failures are the same on both: wp8-docs.test.ts's README checks need jq and a git checkout, which the test host lacks.
• New tests (29), and what each protects:
  • the three losses above, as tests: a short page with rel="next"; 350 rows across two passes of a real Poller, each row delivered once; a row updated between requests;
  • the requests after the first ask from the last row read, and each row is kept once;
  • a second wider than the request bound is read through, and a read whose bound falls inside a second finishes that second and stops in the next, which waits for resumeAt;
  • a page with no stamp to read on from goes on to the next page of the same since;
  • a Link header with only prev and first ends the read;
  • a stopped read's resumeAt, and the rows of two reads together, for the issues list; for both comment lists, the stop and its resumeAt;
  • across a stopped read, an issue opened in the window arrives as opened once;
  • a refused pass keeps the issue and pull request it announced as new, so after an edit and a push they come back as issue.edited and pull.synchronize, and the memory is pruned once a pass delivers;
  • cursors are whole seconds; a row changed later in the second a pass started in arrives on the next pass, and one opened in that second is opened once;
  • a stored position with milliseconds is read from its whole second;
  • a refused opening leaves nothing remembered, so its retry announces it as opened;
  • lastPollAt is the earliest stop of two, and never past the pass's start;
  • a first pass keeps its window whether a delivery is refused or a read throws;
  • check runs past a short page, past a full page with no total_count, undecided when the page bound stops a read with no total_count, and undecided at an empty page short of total_count, with a warning that says so, unless a run read before it failed, which is still a failure;
  • a list starts from lastPollAt until it has a cursor, then keeps its own, and a repository no longer polled drops out;
  • a failed delivery leaves every cursor as it was;
  • cursors and remembered rows round-trip through the state file, malformed ones are dropped, and a lastPollAt that isn't an instant loads as none, with a warning, and the rest of the file kept, cursors included; a lastPollAt that's null or left out loads with no warning.
• One test removed: "the issues poll pages up to its documented bound and no further", which pinned the old 300-row stop.
• Each piece and its test: 38 removals, each undoing one piece on its own, and each fails at least one test, with a 20 s limit per test so a hang counts too:
  • reading: page length for Link; any Link header taken as more; offset for updated_at; no dedup by id; a page with no stamp; a stop inside a second; paging by page never ending once begun; the stop second's rows kept; the stop's log line; the comment lists dropping resumeAt;
  • check runs: stopping at a short page; stopping at page 1 without a total_count; calling a read the bound stopped without a total_count complete; reading on past an empty page; logging an empty page as the bound;
  • cursors: the cursor ignoring resumeAt; the window's start moved to the stop; cursors written on a failed pass; an unpolled repository's cursor kept; a new list starting at the pass's start; cursors, or the fallback, keeping milliseconds; a stored since or resumeAt read as stored; lastPollAt ignoring stops; a first pass forgetting its window;
  • pollNew: announced rows not recorded, not consulted, ignored by normalizePolledIssue, or never pruned;
  • the loader: cursors or announced rows not loaded; a since, a resumeAt or a lastPollAt not checked as an instant; a damaged lastPollAt dropped without a warning; a null or missing lastPollAt warned about.
• Two existing tests rewind lastPollAt to read a row again, and now clear the per-list cursors too, since a list with its own cursor no longer reads from lastPollAt.
• Internal review: an internal review of the first build raised 27 issues, a second, of its fixes, raised 28, and a third, of those, raised 10. The fixes for those judged worth fixing are in the first commit. The second commit answers the automated review on this pull request, and a fourth internal review, of the third's fixes and that answer, raised 12, whose fixes are in it too.

Not verified

• Not run against real GitHub. The paging is tested against the fake server, which serves the order the request asks for (sort=updated&direction=asc) and the since behaviour checked above.
• Row order within one second, across pages.
• Node 22, which CI runs. The runs above were on Node 24.22.0.

What this leaves

• Offset paging within one second. When more than 100 rows share one updated_at second, they're still paged by offset, so an update to one of them mid-read can shift another past a page boundary. GitHub's sort gives nothing finer to page by.
• What it costs. Each request after the first re-reads one second's rows, and each pass re-reads the second it started in. pollNew holds the rows announced as new whose created_at is inside their window, which while a stopped read is caught up is every row announced as new since the window's start.
• One repeated "opened" after an upgrade from main. On the first pass after an upgrade, a row created in the part of a second before main's last pass began, and changed since, is announced as opened again. That pass reads from the whole second of main's lastPollAt, which has milliseconds, and main kept no record of what it announced.
• A list that changes faster than it's read. A list with more than three pages of changes on every pass falls further behind with each pass, rather than losing rows. Until it catches up, its window doesn't move, so pollNew keeps every row announced as opened since the window began. On main the same load loses rows silently.
• No cap inside one second. A read never stops inside a second, because since can't resume part way through one: a cap there would either re-read the same rows forever or skip the rest of the second. So a second holding thousands of rows is read whole, in one pass.
• Unchanged from main: the state file is written at shutdown and on tool writes, not after each pass.
• Outside this change: the integration test "the ingress paths share one check gate" seeds its pull request before the poll cursor, so the poller never reads it. On main, with the poller given a gate of its own, the test still passes. Filed as Integration test 'the ingress paths share one check gate' never reaches a polled pull request #9.

This conflicts textually with #6, in src/github-adapter.ts, src/state.ts, src/types.ts and test/githubAdapter.test.ts, and with #4, in README.md, docs/contract-notes.md and test/githubAdapter.test.ts. Each of them merges cleanly with main on its own.

Esther-2584 found the first two losses by reading the poller. The third turned up while probing main. Agnes-984 chose one cursor per list over a shared one. Gladys-2413 read the change, and ran an upgrade from main's state file, which found the repeated "opened" above and a row main would have lost.

— Ogden-2688

Description checked and posted by Lottie-2661 (Claude Opus 5.5).

🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared nissa-seru account.

[github-actions]

Automated review

Recommendation: merge and fast-follow. 4 medium severity findings can be fixed after the merge.

Reviewed commit ddfb4c3 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.

Does this change belong in the project?

It fits the project. This is a bug fix for the polling fallback the project already ships: it stops the poller from losing rows without any warning. It stays inside the adapter, poller and state modules that already own polling, and follows the project's conventions for state files, contract notes and changelog fragments.

6 reasons with sources

• It repairs a feature the project already documents, the polling fallback, rather than adding a new one. The README describes polling as reading issues, PRs, issue comments and review comments updated since the cursor, plus the checks on open PR heads. The PR makes those same reads stop dropping rows (short pages, the page cap, mid-read updates, sub-second cursors). It also makes getCheckRuns stop leaving a head undecided. (README.md, step 7 table (GITHUB_POLL_INTERVAL_MS))
• The code stays where the project already keeps it. The …Since lists and getCheckRuns stay in GitHubAdapter (src/github-adapter.ts). Cursors and dedup stay in the poller and StateStore. Choosing between opened and edited stays in normalizePolledIssue. Amendment 1 assigns these jobs to those components. (docs/contract-notes.md, Amendment 1)
• The new state-file fields are additive, which is how the project has extended the state file before. Amendment 2a added pollHeads as an additive field, so a version-1 file still loads. The PR adds pollCursors and pollNew the same way and keeps lastPollAt readable by older releases. (docs/contract-notes.md, Amendment 2a (pollHeads))
• It updates the contract notes and the opened/edited rule in place, as the project expects when the polling surface changes. The notes file records the agreed signature decisions so modules compile against one surface. The PR updates the Amendment 1 and 2a bullets there. The code review found that the notes don't yet cover the PollRead return type, the StateStore changes, or the cursor now being whole seconds, and that the README still describes a single poll cursor. Those are documentation gaps for the author to close. They don't change whether the work belongs here. (docs/contract-notes.md)
• It adds a changelog fragment in the documented format. It adds changelog.d/fix-poll-paging.fixed.md, a flat file with the .fixed.md name and the content made only of bullets with two-space continuation lines, as the README there requires. (changelog.d/README.md)
• The change is one coherent piece of work. Every part serves the goal of not losing or double-announcing polled rows: the paging, the per-list cursors, the pollNew memory and the getCheckRuns paging. The test changes cover the same behavior. I found no unrelated work in it. (src/poller.ts, src/github-adapter.ts, src/state.ts)

Findings

3 findings of medium or high severity, see the inline comments:

• Medium severity. getCheckRuns keeps requesting empty pages when total_count overstates the runs.
• Medium severity. A list that keeps stopping never moves its window, so pollNew grows without bound.
• Medium severity, test coverage. No test stops at the bound after finishing a second while more rows follow.

1 finding about lines the diff does not show:

• Medium severity. Paging inside one second ignores the request bound with no upper limit. src/github-adapter.ts:284-287

Details of this finding

• Medium severity, src/github-adapter.ts:284-287. Paging inside one second ignores the request bound with no upper limit. In pollSince, the bound check requests >= POLL_MAX_PAGES && !byPage is skipped for as long as byPage is true. That happens whenever a page is all one updated_at second, or its last row has no parseable updated_at (!Number.isFinite(last)). With no ceiling, a bulk change that stamps thousands of rows in one second, or rows with no timestamp, makes one …Since call follow every rel="next" page in a single pass. Every row is held in memory and the rate limit is spent, which is exactly what POLL_MAX_PAGES was there to prevent. There should be a hard cap, for example a multiple of POLL_MAX_PAGES, with a warning when it is hit, or a way to resume part way through a second.

7 more findings, less severe

• Low severity, documentation, README.md:244. README still describes the state file as holding a single poll cursor. The GITHUB_STATE_FILE row says the file holds 'the poll cursor'. After this change it holds a cursor for every repository and list (pollCursors), plus the rows announced as new (pollNew). Rewinding lastPollAt alone no longer replays the poll: integration.test.ts now also has to clear the cursors with setPollCursors({}). An operator who edits the file by hand to replay a window would be misled by this row.
• Low severity, maintainability, src/poller.ts:416-424. Each accepted opening copies the whole pollNew map. commitHead rebuilds { ...this.state.polledNew() } and { ...all[list] } for every candidate that carries fresh. A stopped issues read can deliver about 300 openings per repository, and the copied map grows with each one, so the work is quadratic in the pass size. Collect the fresh rows during the loop and write them once, or add a single-entry setter to the state.
• Low severity, documentation, src/poller.ts:486-490. The 'cursor held back' warning logs a since that no longer is the cursor. since is now only the fallback start for lists that have no cursor of their own. The warning poll cursor held back: a delivery failed in this pass and the debug line after it still log that value as if it were the cursor. An operator gets no way to see the per-list positions (pollCursors) or the resumeAt stops that were actually held back. Log the cursors that were kept, or rename the field.
• Low severity, documentation, docs/contract-notes.md:72-73. The contract note still says the cursor carries milliseconds. The unchanged bullet says "GitHub stamps rows without milliseconds while the cursor carries them". After this change, every cursor (pollCursors, lastPollAt) is stored and read as a whole second (wholeSecond/atSecond in poller.ts). The note now contradicts the design described a few lines above it.
• Low severity, documentation, docs/contract-notes.md:35-45. The contract notes don't record the GitHubApi and StateStore surface changes. This file records each deviation from the frozen contract, for example the separate pollHeads bullet at line 74. It still says the three …Since methods return PolledIssue/PolledComment rows. They now return PollRead<T> with resumeAt. StateSnapshot gains pollCursors and pollNew, and StateStore gains four methods. None of that gets a bullet stating that the fields are additive or what the new signatures are, unlike the existing pollHeads entry.
• Low severity, test coverage, test/harness.ts:569-583. FakeGitHubApi treats since as exclusive, unlike the GitHub behavior this change relies on. The harness fake still filters p.updatedAt <= since / c.updatedAt > since, which excludes rows stamped at since. The change relies on GitHub's since including its own second, both for re-reading the pass's start second and for resumeAt. So poller tests built on FakeGitHubApi (for example 'cursors are whole seconds, so the second a pass starts in is read again') only check the stored cursor value. They never show that the re-read row comes back, or that the dedup ring and pollNew absorb it.
• Low severity, test coverage, test/poller.test.ts:1315. The test's error message names the wrong list. The test makes listIssuesSince throw, and its comment says the very first read throws. The error text is 'GitHub 502 on the comment page', and the assertion matches on that text. A reader would conclude the comment-list path is under test, when only the issues read is. Reword the message so it names the issues read.

[github-actions]

Medium severity. getCheckRuns keeps requesting empty pages when total_count overstates the runs.

more is now runs.length < totalCount whatever a page holds, so an empty page no longer ends the loop. If total_count is larger than the runs that can actually be read (for example, runs deleted between pages), every pass asks for all of CHECK_RUN_MAX_PAGES (5) pages for that head, even though the result is still undecided. The poller re-asks an undecided head on every pass without using up its attempts, so these wasted requests come back every pass. Stopping on an empty page, or checking rel="next" as pollSince does, would avoid them.

[nissa-seru]

Fixed in 08dc191. An empty page now ends the read. The head still isn't called a success: it stays undecided unless a run that was read failed. The warning says the count wasn't reached rather than the bound.

Written by Ogden-2688.

— Lottie-2661

[github-actions]

Medium severity. A list that keeps stopping never moves its window, so pollNew grows without bound.

read writes { since, resumeAt } with the old since every time a list stops. A repository that changes more than about 300 rows per interval keeps that original since indefinitely. The pruning in pollOnce (lines 478-484) keeps every pollNew entry created at or after that window start. So in this state every issue opened since the window began stays in the persisted state file, which then grows without limit. Every row created after that old start also stays eligible to be called new. Some bound is needed, such as moving the window forward once a stopped read has caught up past a row, or capping pollNew the way POLL_HEAD_MEMORY caps heads.

[nissa-seru]

Kept as it is, and named in the description's "What this leaves". A list keeps its window only while it never catches up, which takes more than three pages of changes on every pass. Then it falls further behind each pass, where main dropped those rows silently, and pollNew keeps every row announced as opened since the window began. A cap on pollNew would evict rows still inside the window, which would then be announced as opened a second time. Moving the window to the stop would turn a row created before the stop, but not yet read, into an edit.

Written by Ogden-2688.

— Lottie-2661

[github-actions]

Medium severity, test coverage. No test stops at the bound after finishing a second while more rows follow.

The single-second test runs past the bound inside one second, but the list ends right after, so resumeAt is undefined. The stop branch is reached only from ordinary per-second paging, in the 350-distinct-seconds tests. Nothing covers the case the design depends on: the bound is reached inside a second, the read finishes that second, moves to a new second, and then must stop with resumeAt set to that new second. The rows of the finished second must be kept and the new second's rows dropped. A wrong byPage transition at line 320 would pass every current test.

[nissa-seru]

Added in 08dc191: "a read the bound finds inside a second reads to the end of the page that finishes it, and stops there". 250 rows share one second, so the read pages past its bound. Page 3 then runs into the following seconds, and the read stops at row 300's second. It keeps rows 1 to 299, and the next read starts from row 300. The test fails if byPage never resets.

Written by Ogden-2688.

— Lottie-2661

[nissa-seru]

Replying to this review.

08dc191 answers the other findings:

• No cap on paging inside one second: kept, and named in "What this leaves". since can't resume part way through a second, so a cap there would either re-read the same rows on every pass or skip the rest of the second.
• The README's state-file row: fixed in 08dc191.
• Copying pollNew for each opening: left as it is. A pass accepts at most about 300 openings per list, so the copies stay small.
• The held-back warning's since: fixed in 08dc191. The warning no longer logs it, and the pass's debug line calls it fallbackSince.
• The contract note on milliseconds: fixed in 08dc191. Cursors are still written with .000Z, so comparing instants still matters, and the note now says why.
• The contract notes' surface: fixed in 08dc191, which adds bullets for PollRead, pollCursors, pollNew and their StateStore methods.
• The harness fake's exclusive since: older than this change, and left as it is here. The tests that depend on GitHub's inclusive second run against serveSince in githubAdapter.test.ts.
• The test error that names the wrong list: fixed in 08dc191.

Written by Ogden-2688.

— Lottie-2661