fix(github): page installation repositories from 1, and stop when a page is empty - #2906
Open
sujeito-operator wants to merge 1 commit into
Open
Conversation
…age is empty fetchAllRepos asks for page 1, then keeps requesting `len(repos) / 100` while `len(repos) < *result.TotalCount`. GitHub's pagination is 1-indexed, so after the first 100 repositories that expression is 1 again - page 1 is fetched a second time, and every subsequent page index trails the real one by one. An installation with 250 repositories returns 300: page 1 twice, page 2 once, and page 3 never. The user sees duplicates in the picker and cannot connect the repositories that fell off the end, which looks like the repository simply is not there. The loop also has no way to end other than reaching TotalCount. If an installation reports more repositories than it will actually hand out - a page comes back empty while len(repos) is still short of TotalCount - the condition stays true forever and the loop keeps calling the GitHub API inside the HTTP request that triggered it. `*result.TotalCount` is dereferenced without a nil check as well. Walk the pages with an explicit 1-indexed counter, stop as soon as a page yields no repositories, and treat a missing total_count as "that was the last page". Tests drive fetchAllRepos against an httptest server that records which pages were requested, so no GitHub credentials are needed. Against the current code the multi-page case drops acme/repo-201..250 and the inconsistent-total_count case does not terminate; both pass with this change. Related to l3montree-dev#2572, which is about the same repository-listing path but asks for orphaned-installation feedback and deletion on top - not addressed here. Signed-off-by: Sujeito Operator <operator@sujeito.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fetchAllReposinintegrations/githubint/github_client.gopages the installation's repositories like this:GitHub's pagination is 1-indexed, so
len(repos) / 100is the page we just read, not the next one.What that does
For an installation with 250 repositories (three pages of 100/100/50):
len(repos)Page 1 is fetched twice and page 3 is never fetched. The caller gets 300 entries for 250 repositories: 100 duplicates, and
acme/repo-201..250missing entirely. In the UI that reads as duplicated rows in the repository picker plus repositories that cannot be connected at all — which is easy to mistake for the repository not being visible to the installation.And it can fail to terminate
The only exit is reaching
TotalCount. If an installation reports more repositories than it will actually hand out, a page comes back empty whilelen(repos)is still short ofTotalCount, the condition stays true, and the loop keeps calling the GitHub API — inside the HTTP request that triggered the listing.*result.TotalCountis also dereferenced without a nil check.The change
Walk pages with an explicit 1-indexed counter, stop as soon as a page yields no repositories, and treat a missing
total_countas "that was the last page". Same signature, same return value, no behaviour change for installations that fit on one page.Verification
integrations/githubint/github_client_test.godrivesfetchAllReposagainst anhttptestserver that records which page numbers were requested, so no GitHub credentials or network access are needed. Three cases: single page, 250 repositories across three pages, and a server whosetotal_countoverstates what it returns.Against unmodified
mainthe new tests fail exactly as described — the multi-page case reportsacme/repo-201..250missing, and the inconsistent-total_countcase hits the test's 10s guard without terminating. With this change all three pass in 0.01s.I ran the full suite on
mainand on this branch and the results are identical: 31 packagesok, and the same 9 pre-existing failures indatabase/repositories(TestAutoTenantScope*) andtests(TestGitleaksVulnPathThroughRealBackend), which need a Docker/testcontainers runtime this machine does not have.gofmtandgo vetare clean.golangci-lintreports the same 7 pre-existingstaticcheckfindings before and after, all ingithub_integration.go, which this PR does not touch; 0 findings in either file it does touch.Scope
This is only the paging bug. #2572 concerns the same repository-listing path but asks for orphaned-installation feedback and deletion on top of graceful degradation — I have deliberately not touched that here, since the hotfix in d688ab5 already keeps a dead installation from breaking the healthy ones, and the remaining parts of that issue are a UI/API surface question rather than this loop. Happy to pick that up separately if it would help.
One judgement call worth flagging: I kept
PerPage: 100as a named constant rather than making it configurable, since nothing else in the file parameterises it. Say the word if you would rather it stayed a literal.