Skip to content

fix: stop unbounded pagination in check_rate_limit when run exceeds threshold #48972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jul 29, 2026
+14 −10
Merged

fix: stop unbounded pagination in check_rate_limit when run exceeds threshold #48972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a78ec5
Initial plan
Copilot Jul 29, 2026
90a70a5
fix: stop unbounded pagination in check_rate_limit when run is older …
Copilot Jul 29, 2026
07a37bd
Merge branch 'main' into copilot/aw-failures-fix-rate-limit-check
github-actions[bot] Jul 29, 2026
27fbb3a
fix: reorder test fixture to newest-first and add pagination assertion
Copilot Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions actions/setup/js/check_rate_limit.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,12 @@ async function main() {
break;
}

// Skip if run is older than the time window
// Stop if run is older than the time window (runs are newest-first)
const runCreatedAt = new Date(run.created_at);
if (runCreatedAt < thresholdTime) {
core.info(` Skipping run ${run.id} - created before threshold (${run.created_at})`);
continue;
core.info(` Stopping pagination - run ${run.id} created before threshold (${run.created_at})`);
hasMore = false;

@github-actions github-actions Bot Jul 29, 2026

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.

Correct fix logically, but no regression test was added for this exact incident (unbounded pagination when a run predates the threshold) — the bug that caused a 37% failure rate for 6h will have no test guarding against reintroduction.

💡 Add a targeted test

The fix relies on the undocumented-in-code assumption that listWorkflowRuns returns runs newest-first (true by API default, but not enforced/asserted anywhere). A test mocking a paginated response where an early run in page 1 is older than thresholdTime, followed by mock pages that would need many more calls if pagination continued, would catch a future regression to continue and also validate this ordering assumption. Consider asserting github.rest.actions.listWorkflowRuns is called exactly once in that scenario.

break;

@github-actions github-actions Bot Jul 29, 2026

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.

[/tdd] The existing test "should exclude runs older than the time window" does not assert that pagination stops — it only checks the count. With the old continue the test would still pass, meaning this regression can silently reappear.

💡 Suggested regression test 
it("should stop pagination when a run predates the threshold", async () => {
  // First page has one stale run (older than window)
  mockGithub.rest.actions.listWorkflowRuns
    .mockResolvedValueOnce({
      data: {
        workflow_runs: [{
          id: 999,
          run_number: 1,
          created_at: new Date(Date.now() - 120 * 60 * 1000).toISOString(),
          actor: { login: "test-user" },
          status: "completed",
        }],
      },
    })
    .mockResolvedValue({ data: { workflow_runs: [] } });

  await checkRateLimit.main();

  // Must only call once — page 2 should never be fetched
  expect(mockGithub.rest.actions.listWorkflowRuns).toHaveBeenCalledTimes(1);
  expect(mockCore.info).toHaveBeenCalledWith(
    expect.stringContaining("Stopping pagination")
  );
});

Without this, a future continue reintroduction would go undetected.

@copilot please address this.

}

// Check if run is by the same actor
Expand Down
17 changes: 10 additions & 7 deletions actions/setup/js/check_rate_limit.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,21 +196,22 @@ describe("check_rate_limit", () => {
const twoHoursAgo = new Date(Date.now() - 120 * 60 * 1000);
const recentTime = new Date(Date.now() - 10 * 60 * 1000);

mockGithub.rest.actions.listWorkflowRuns.mockResolvedValue({
// API returns newest-first: recent run is listed before the old run
mockGithub.rest.actions.listWorkflowRuns.mockResolvedValueOnce({
data: {
workflow_runs: [
{
id: 111111,
run_number: 1,
created_at: twoHoursAgo.toISOString(),
id: 222222,
run_number: 2,
created_at: recentTime.toISOString(),
actor: { login: "test-user" },
event: "workflow_dispatch",
status: "completed",
},
{
id: 222222,
run_number: 2,
created_at: recentTime.toISOString(),
id: 111111,
run_number: 1,
created_at: twoHoursAgo.toISOString(),
actor: { login: "test-user" },
event: "workflow_dispatch",
status: "completed",
Expand All @@ -223,6 +224,8 @@ describe("check_rate_limit", () => {

expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true");
expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Total recent runs in last 60 minutes: 1"));
// Once the old run is found, no further pages should be fetched
expect(mockGithub.rest.actions.listWorkflowRuns).toHaveBeenCalledTimes(1);
});

it("should exclude the current run from the count", async () => {
Expand Down
Loading