NextChapterSoftware/ec2-action-builder

do...while loop can create a scenario where multiple and rapid API requests happen

Closed this issue · 7 comments

According to chatgpt, getRunnerWithLabels method in github.ts can end up in a rapid loop causing lots of API requests.

You can see the relevant part of the conversation at the bottom:

https://chatgpt.com/share/66e9a38a-e2e0-8012-99e8-6b60a1023201

Rapid loop code illustration:

async getRunnerWithLabels(labels: string[]) {
  const octokit = github.getOctokit(this.config.githubToken);
  var done = false;
  do {
    // Makes an API request
    const runners = await octokit.rest.actions.listSelfHostedRunnersForRepo({ ... });
    done = !Boolean(runners.data.total_count);

    // Attempts to find a matching runner
    const matches = _.filter(runners.data.runners, searchLabels);
    return matches.length > 0 ? matches[0] : null;

    // No delay here, so the loop continues rapidly
  } while (done);
  return null;
}

Impact
High API Request Rate: This loop can cause the action to make multiple API requests per second to GitHub.
Rate Limit Exceeded: GitHub has API rate limits, even for authenticated requests. Excessive requests can lead to throttling.
Unnecessary Load: The rapid requests are unnecessary and can be reduced by introducing a delay.

Conclusion
The high rate of API requests to GitHub is caused by the lack of delay in the do...while loop within the getRunnerWithLabels method in github.ts. By introducing a delay in this loop, you can control the rate of API requests and prevent exceeding GitHub's API rate limits.

Chatgpt recommends using a delay like:

async getRunnerWithLabels(labels: string[]) {
  const octokit = github.getOctokit(this.config.githubToken);
  var done = false;
  do {
    try {
      const runners = await octokit.rest.actions.listSelfHostedRunnersForRepo(
        {
          owner: github.context.repo.owner,
          repo: github.context.repo.repo,
        }
      );
      done = !Boolean(runners.data.total_count);

      const searchLabels = {
        labels: labels.map(function (label) {
          return { name: label };
        }),
      };

      const matches = _.filter(runners.data.runners, searchLabels);
      if (matches.length > 0) {
        return matches[0];
      } else {
        // Introduce a delay before the next iteration
        await new Promise((resolve) => setTimeout(resolve, 1000)); // 1-second delay
      }
    } catch (error) {
      core.error(`Failed to list github runners: ${error}`);
      throw error;
    }
  } while (!done);
  return null;
}

Ah I see. Is there value in having this timeout be customizable using the same parameter discussed here ?

I am working on adding a gh_api_retry_delay param for the other two calls. We should be able to incorporate this into almost all GH API calls. I'll review the other calls to make sure we don't end up in this situation again.

Ah I see. Is there value in having this timeout be customizable using the same parameter discussed here ?

Yes, I think that would be great!

Ok I reviewed the code and the do-while loop inside of getRunnerWithLabels is not needed. It was leftover code before adding the wait and retry logic to the polling function. I am testing my changes right now and if all goes well I'll be pushing a new release later this evening.

#33 was merged to main

Release 1.8 is out with this change

I tested it with my setup and it works as expected. Let me know if you face any issues otherwise please close this GitHub issue

We're no longer hitting github token limits. Thank you for this!