[ISD-3791] Fix: Add timeouts for ghapi calls (#586)

Author: yhaliaw

github-runner-manager/src/github_runner_manager/github_client.py

@@ -9,7 +9,7 @@
 import logging
 from datetime import datetime
 from typing import Callable, ParamSpec, TypeVar
-from urllib.error import HTTPError
+from urllib.error import HTTPError, URLError
 
 import requests
 
@@ -39,6 +39,8 @@
 
 logger = logging.getLogger(__name__)
 
+TIMEOUT_IN_SECS = 60
+
 
 class GithubRunnerNotFoundError(Exception):
     """Represents an error when the runner could not be found on GitHub."""
@@ -77,6 +79,7 @@ def wrapper(*args: ParamT.args, **kwargs: ParamT.kwargs) -> ReturnT:
         """
         try:
             return func(*args, **kwargs)
+        # The ghapi module uses urllib. The HTTPError and URLError are urllib exceptions.
         except HTTPError as exc:
             if exc.code in (401, 403):
                 if exc.code == 401:
@@ -86,9 +89,15 @@ def wrapper(*args: ParamT.args, **kwargs: ParamT.kwargs) -> ReturnT:
                 raise TokenError(msg) from exc
             logger.warning("Error in GitHub request: %s", exc)
             raise PlatformApiError from exc
+        except URLError as exc:
+            logger.warning("General error in GitHub request: %s", exc)
+            raise PlatformApiError from exc
         except RequestException as exc:
             logger.warning("Error in GitHub request: %s", exc)
             raise PlatformApiError from exc
+        except TimeoutError as exc:
+            logger.warning("Timeout in GitHub request: %s", exc)
+            raise PlatformApiError from exc
 
     return wrapper
 
@@ -124,11 +133,11 @@ def get_runner(self, path: GitHubPath, prefix: str, runner_id: int) -> SelfHoste
         try:
             if isinstance(path, GitHubRepo):
                 raw_runner = self._client.actions.get_self_hosted_runner_for_repo(
-                    path.owner, path.repo, runner_id
+                    path.owner, path.repo, runner_id, timeout=TIMEOUT_IN_SECS
                 )
             else:
                 raw_runner = self._client.actions.get_self_hosted_runner_for_org(
-                    path.org, runner_id
+                    path.org, runner_id, timeout=TIMEOUT_IN_SECS
                 )
         except HTTP404NotFoundError as err:
             raise GithubRunnerNotFoundError from err
@@ -164,6 +173,7 @@ def list_runners(self, path: GitHubPath, prefix: str) -> list[SelfHostedRunner]:
                     owner=path.owner,
                     repo=path.repo,
                     per_page=100,
+                    timeout=TIMEOUT_IN_SECS,
                 )
                 for item in page["runners"]
             ]
@@ -179,6 +189,7 @@ def list_runners(self, path: GitHubPath, prefix: str) -> list[SelfHostedRunner]:
                     num_of_pages + 1,
                     org=path.org,
                     per_page=100,
+                    timeout=TIMEOUT_IN_SECS,
                 )
                 for item in page["runners"]
             ]
@@ -217,6 +228,7 @@ def get_runner_registration_jittoken(
                 name=instance_id.name,
                 runner_group_id=1,
                 labels=labels,
+                timeout=TIMEOUT_IN_SECS,
             )
         elif isinstance(path, GitHubOrg):
             # We cannot cache it in here, as we are running in a forked process.
@@ -227,6 +239,7 @@ def get_runner_registration_jittoken(
                 name=instance_id.name,
                 runner_group_id=runner_group_id,
                 labels=labels,
+                timeout=TIMEOUT_IN_SECS,
             )
         else:
             assert_never(token)
@@ -249,7 +262,7 @@ def _get_runner_group_id(self, org: GitHubOrg) -> int:
             "Authorization": f"Bearer {self._token}",
             "X-GitHub-Api-Version": "2022-11-28",
         }
-        response = requests.get(url, headers=headers, timeout=30)
+        response = requests.get(url, headers=headers, timeout=TIMEOUT_IN_SECS)
         response.raise_for_status()
         data = response.json()
         try:
@@ -282,11 +295,13 @@ def delete_runner(self, path: GitHubPath, runner_id: int) -> None:
                     owner=path.owner,
                     repo=path.repo,
                     runner_id=runner_id,
+                    timeout=TIMEOUT_IN_SECS,
                 )
             else:
                 self._client.actions.delete_self_hosted_runner_from_org(
                     org=path.org,
                     runner_id=runner_id,
+                    timeout=TIMEOUT_IN_SECS,
                 )
         # The function delete_self_hosted_runner fails in GitHub if the runner does not exist,
         # so we do not have to worry about that.
@@ -310,7 +325,12 @@ def get_job_info_by_runner_name(
         Returns:
             Job information.
         """
-        paged_kwargs = {"owner": path.owner, "repo": path.repo, "run_id": workflow_run_id}
+        paged_kwargs = {
+            "owner": path.owner,
+            "repo": path.repo,
+            "run_id": workflow_run_id,
+            "timeout": 60,
+        }
         try:
             for wf_run_page in paged(
                 self._client.actions.list_jobs_for_workflow_run, **paged_kwargs
@@ -353,6 +373,7 @@ def get_job_info(self, path: GitHubRepo, job_id: str) -> JobInfo:
                 owner=path.owner,
                 repo=path.repo,
                 job_id=job_id,
+                timeout=TIMEOUT_IN_SECS,
             )
         except HTTPError as exc:
             if exc.code == 404:

github-runner-manager/tests/unit/test_github_client.py

@@ -517,7 +517,7 @@ def raise_for_status(self):
 
     instance_id = InstanceID.build("test-runner")
 
-    def _mock_generate_runner_jitconfig_for_org(org, name, runner_group_id, labels):
+    def _mock_generate_runner_jitconfig_for_org(org, name, runner_group_id, labels, timeout):
         """Mocked generate_runner_jitconfig_for_org."""
         assert org == "theorg"
         assert name == instance_id.name