Skip to content

perf: make getSupportedBranches faster - #209

Open
dsanders11 wants to merge 2 commits into
mainfrom
perf/faster-supported-branches
Open

perf: make getSupportedBranches faster#209
dsanders11 wants to merge 2 commits into
mainfrom
perf/faster-supported-branches

Conversation

@dsanders11

Copy link
Copy Markdown
Member

Follow-up to #207, which added more calls of getSupportedBranches to the common code paths. Same general change as electron/sudowoodo#419, just adapted to this codebase.

Validated locally as only taking around 800ms now to fetch supported branches, while from Datadog log entries it looks like it's currently taking 25+ seconds per call.

Assisted-by: GPT-5.6 Sol
@dsanders11
dsanders11 requested review from a team as code owners September 3, 2026 21:53

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any concrete bugs. Because it replaces a well-established REST-based branch lookup with a new GraphQL query whose fuzzy -x-y query matching, dropped protected: true filter, and hardcoded first: 100 (no pagination) are hard to fully validate without hitting the live GitHub API, a human look would still be worthwhile before merging.

What was reviewed: the rewritten getSupportedBranches GraphQL query and its input validation, the removal of the old REST protected: true filter (branches no longer need to be protected to count as "supported"), the unpaginated first: 100 fetch versus the old unbounded github.paginate, and the updated call sites in chromium-handler.ts, node-handler.ts, and get-target-branch-labels.ts plus their test coverage.

Extended reasoning...

Overview

This PR replaces the REST-based github.repos.listBranches/getBranch pagination in getSupportedBranches with a single GraphQL query against repository.refs, changes the function to be async and take an Octokit client directly, adds numSupportedVersions bounds validation, and introduces a lightweight Branch type used across chromium-handler.ts, node-handler.ts, and get-target-branch-labels.ts. Tests were updated accordingly, including a new get-supported-branches.spec.ts that mocks octokit.graphql.

Security risks

No injection, auth-bypass, or data-exposure concerns: the GraphQL query uses parameterized variables ($owner, $repo, $branchQuery), and the repo/owner are hardcoded constants, not user input. The numSupportedVersions validation is a reasonable input-sanity check, not a security boundary.

Level of scrutiny

This warrants more than a cursory pass because it changes core semantics of a function used to decide which branches get rolled/labeled, not just a performance tweak. Two behavior changes stand out from reading the diff directly: (1) the old REST path always fetched branches with protected: true, so only protected branches were ever eligible to be "supported"; the new GraphQL query has no such constraint, so any branch matching the release-name regex (protected or not) is now included. (2) the query is capped at first: 100 with no pageInfo/cursor follow-up, unlike the old github.paginate which fetched every branch. Both were already surfaced as candidate issues and investigated/ruled out in this run's bug-hunting pass, but I could not independently confirm GitHub's exact fuzzy-match semantics for the refs(query:) argument (e.g., whether it requires a literal substring or a subsequence, which affects whether legacy N-M-x freeze branches would still match -x-y) since outbound network access to the GitHub API is blocked in this sandbox.

Other factors

Test coverage for the new function is reasonable (mocks graphql, covers filtering and count-limiting), but since the mocks stand in for the live GraphQL API, they can't validate the real fuzzy-match behavior of refs(query: ...), which is exactly the part of this change that's hardest to reason about statically. Given that, plus the semantic changes noted above touching a function that gates which branches get rolled, I think a human with access to test against the live API (as the PR author says they did, citing an ~800ms measurement) is warranted before merging, even though no independent findings are being raised here beyond what was already investigated.

Assisted-by: GPT-5.6 Sol
@dsanders11

Copy link
Copy Markdown
Member Author

Added pagination just to be safe - the longer term goal here is to switch to using /schedule.json as the source of truth for supported branches entirely, but it needs to be improved a bit to ensure new branches show up in a timely manner.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. The follow-up commit adding manual GraphQL pagination directly addresses the earlier concern about only fetching a single page (100) of branches, and is now covered by a dedicated pagination test. Because this replaces the REST-based protected-branch listing with a GraphQL name-pattern query across several call sites (chromium-handler, node-handler, roll.ts), a human look is still worthwhile.

What was reviewed:

  • Pagination loop in getSupportedBranches (cursor handling, hasNextPage/endCursor) and its new test coverage for multi-page results.
  • Dropping the protected: true filter in favor of the -x-y GraphQL search plus local regex filtering — checked whether this could admit unprotected/non-release branches; ruled out.
  • Whether the hardcoded branchQuery: '-x-y' server-side filter could exclude valid N-M-x branches still accepted by the local regex — ruled out.
  • Signature change from sync getSupportedBranches(branches, n) to async getSupportedBranches(github, n) and the updated call sites/tests.
Extended reasoning...

This PR replaces REST-based (listBranches + paginate) discovery of Electron's supported release branches with a GraphQL query against electron/electron's refs, changing getSupportedBranches from a synchronous filter over a caller-supplied branch list to an async function that itself queries and paginates GitHub. It touches chromium-handler.ts, node-handler.ts, get-target-branch-labels.ts, roll.ts, and types.ts (new Branch type replacing two octokit REST response types), plus corresponding test updates across three spec files.

Security risk is low: the GraphQL query uses parameterized variables (owner, repo, branchQuery, cursor), not string interpolation, so there's no injection concern, and the repo/owner are hardcoded rather than user-influenced.

A prior version of this PR was reviewed and raised a pagination concern — the original GraphQL query fetched only a single page (first: 100) with no cursor handling, which could silently drop supported branches once the repo has more than 100 matching refs. The commit pushed after that review (6dba230, "chore: manually paginate GraphQL query") adds a cursor-based pagination loop and a new test (fetches every page of release branches) that exercises it, directly resolving that concern. This run's bug hunt investigated the removal of the old protected: true filter and a possible mismatch between the server-side -x-y search term and the locally-accepted N-M-x regex pattern, and ruled both out (no bugs reported this run).

Given the change alters a widely-used piece of branch-discovery logic (now called from multiple handlers with a different async signature) and swaps out a protection-based filter for a name-pattern-based one, I think the correct scrutiny level is a human sanity check even though the automated review found nothing actionable — this is a meaningful behavioral/architectural change rather than a mechanical one, even though it's now well covered by tests and the identified pagination gap has been closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant