From 7fb46d00f90555ffd35011df821b9b388ec329d9 Mon Sep 17 00:00:00 2001 From: yhaliaw <43424755+yhaliaw@users.noreply.github.com> Date: Fri, 27 Jun 2025 16:19:26 +0800 Subject: [PATCH 1/6] Add timeouts for ghapi calls --- .../src/github_runner_manager/github_client.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/github_client.py b/github-runner-manager/src/github_runner_manager/github_client.py index dba153df10..f1c238be57 100644 --- a/github-runner-manager/src/github_runner_manager/github_client.py +++ b/github-runner-manager/src/github_runner_manager/github_client.py @@ -89,6 +89,9 @@ def wrapper(*args: ParamT.args, **kwargs: ParamT.kwargs) -> ReturnT: 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 +127,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=60 ) else: raw_runner = self._client.actions.get_self_hosted_runner_for_org( - path.org, runner_id + path.org, runner_id, timeout=60 ) except HTTP404NotFoundError as err: raise GithubRunnerNotFoundError from err @@ -164,6 +167,7 @@ def list_runners(self, path: GitHubPath, prefix: str) -> list[SelfHostedRunner]: owner=path.owner, repo=path.repo, per_page=100, + timeout=60, ) for item in page["runners"] ] @@ -179,6 +183,7 @@ def list_runners(self, path: GitHubPath, prefix: str) -> list[SelfHostedRunner]: num_of_pages + 1, org=path.org, per_page=100, + timeout=60, ) for item in page["runners"] ] @@ -217,6 +222,7 @@ def get_runner_registration_jittoken( name=instance_id.name, runner_group_id=1, labels=labels, + timeout=60, ) elif isinstance(path, GitHubOrg): # We cannot cache it in here, as we are running in a forked process. @@ -227,6 +233,7 @@ def get_runner_registration_jittoken( name=instance_id.name, runner_group_id=runner_group_id, labels=labels, + timeout=60, ) else: assert_never(token) @@ -282,11 +289,13 @@ def delete_runner(self, path: GitHubPath, runner_id: int) -> None: owner=path.owner, repo=path.repo, runner_id=runner_id, + timeout=60, ) else: self._client.actions.delete_self_hosted_runner_from_org( org=path.org, runner_id=runner_id, + timeout=60, ) # 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 +319,7 @@ 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 +362,7 @@ def get_job_info(self, path: GitHubRepo, job_id: str) -> JobInfo: owner=path.owner, repo=path.repo, job_id=job_id, + timeout=60, ) except HTTPError as exc: if exc.code == 404: From 9ea21a443ed9991bc06c2c75b8a35cf4f2cd7961 Mon Sep 17 00:00:00 2001 From: yhaliaw <43424755+yhaliaw@users.noreply.github.com> Date: Mon, 30 Jun 2025 10:39:10 +0800 Subject: [PATCH 2/6] Add catching of URLError for urllib --- .../src/github_runner_manager/github_client.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/github_client.py b/github-runner-manager/src/github_runner_manager/github_client.py index f1c238be57..3dba0d785a 100644 --- a/github-runner-manager/src/github_runner_manager/github_client.py +++ b/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 @@ -77,6 +77,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,6 +87,9 @@ 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 @@ -319,7 +323,12 @@ def get_job_info_by_runner_name( Returns: Job information. """ - paged_kwargs = {"owner": path.owner, "repo": path.repo, "run_id": workflow_run_id, "timeout": 60} + 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 From b1db3736db382408c02fa9c9d58fab5618e421ab Mon Sep 17 00:00:00 2001 From: yhaliaw <43424755+yhaliaw@users.noreply.github.com> Date: Tue, 1 Jul 2025 10:11:39 +0800 Subject: [PATCH 3/6] Fix unit test --- github-runner-manager/tests/unit/test_github_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github-runner-manager/tests/unit/test_github_client.py b/github-runner-manager/tests/unit/test_github_client.py index 33989a984c..d3fdcba74f 100644 --- a/github-runner-manager/tests/unit/test_github_client.py +++ b/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 From 8a8b69e439abefb282ad5f365dcb365ead5059e1 Mon Sep 17 00:00:00 2001 From: yhaliaw <43424755+yhaliaw@users.noreply.github.com> Date: Tue, 1 Jul 2025 10:19:59 +0800 Subject: [PATCH 4/6] Add changelog --- docs/changelog.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/changelog.md b/docs/changelog.md index 68d45edcbd..64f6a7a17d 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,6 +2,10 @@ This changelog documents user-relevant changes to the GitHub runner charm. +## 2025-07-01 + +- Fix a possible process leak. + ## 2025-06-26 - Fix a process leak internal to the charm. From b199d867f775f1903eac1414dd71602f37726a03 Mon Sep 17 00:00:00 2001 From: yhaliaw <43424755+yhaliaw@users.noreply.github.com> Date: Tue, 1 Jul 2025 14:08:12 +0800 Subject: [PATCH 5/6] Remove the changelog entry --- docs/changelog.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 64f6a7a17d..68d45edcbd 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,10 +2,6 @@ This changelog documents user-relevant changes to the GitHub runner charm. -## 2025-07-01 - -- Fix a possible process leak. - ## 2025-06-26 - Fix a process leak internal to the charm. From a54245c8bbd1e0c4459476356405a2480ae686e8 Mon Sep 17 00:00:00 2001 From: yhaliaw <43424755+yhaliaw@users.noreply.github.com> Date: Tue, 1 Jul 2025 14:10:46 +0800 Subject: [PATCH 6/6] Use constant for timeout --- .../github_runner_manager/github_client.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/github_client.py b/github-runner-manager/src/github_runner_manager/github_client.py index 3dba0d785a..d3ee48c83f 100644 --- a/github-runner-manager/src/github_runner_manager/github_client.py +++ b/github-runner-manager/src/github_runner_manager/github_client.py @@ -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.""" @@ -131,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, timeout=60 + 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, timeout=60 + path.org, runner_id, timeout=TIMEOUT_IN_SECS ) except HTTP404NotFoundError as err: raise GithubRunnerNotFoundError from err @@ -171,7 +173,7 @@ def list_runners(self, path: GitHubPath, prefix: str) -> list[SelfHostedRunner]: owner=path.owner, repo=path.repo, per_page=100, - timeout=60, + timeout=TIMEOUT_IN_SECS, ) for item in page["runners"] ] @@ -187,7 +189,7 @@ def list_runners(self, path: GitHubPath, prefix: str) -> list[SelfHostedRunner]: num_of_pages + 1, org=path.org, per_page=100, - timeout=60, + timeout=TIMEOUT_IN_SECS, ) for item in page["runners"] ] @@ -226,7 +228,7 @@ def get_runner_registration_jittoken( name=instance_id.name, runner_group_id=1, labels=labels, - timeout=60, + timeout=TIMEOUT_IN_SECS, ) elif isinstance(path, GitHubOrg): # We cannot cache it in here, as we are running in a forked process. @@ -237,7 +239,7 @@ def get_runner_registration_jittoken( name=instance_id.name, runner_group_id=runner_group_id, labels=labels, - timeout=60, + timeout=TIMEOUT_IN_SECS, ) else: assert_never(token) @@ -260,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: @@ -293,13 +295,13 @@ def delete_runner(self, path: GitHubPath, runner_id: int) -> None: owner=path.owner, repo=path.repo, runner_id=runner_id, - timeout=60, + timeout=TIMEOUT_IN_SECS, ) else: self._client.actions.delete_self_hosted_runner_from_org( org=path.org, runner_id=runner_id, - timeout=60, + 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. @@ -371,7 +373,7 @@ def get_job_info(self, path: GitHubRepo, job_id: str) -> JobInfo: owner=path.owner, repo=path.repo, job_id=job_id, - timeout=60, + timeout=TIMEOUT_IN_SECS, ) except HTTPError as exc: if exc.code == 404: