From d9a4983cfc2c4ccde28c64bd9b64a7eb7103ccef Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 22 Jan 2026 16:24:34 +0200 Subject: [PATCH 1/6] fix(github-api): handle authentication errors in add_api_users_to_auto_verified_and_merged_users Previously, if a GitHub API token failed authentication with a 401 error, the GithubException was not caught, causing the server to crash. Added try/except block to catch GithubException and log a warning instead of crashing, allowing the server to continue with other valid tokens. --- webhook_server/libs/github_api.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index b24cdd1b..2481906b 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -595,14 +595,24 @@ async def process(self) -> Any: def add_api_users_to_auto_verified_and_merged_users(self) -> None: apis_and_tokens = get_apis_and_tokes_from_config(config=self.config) - for _api, _ in apis_and_tokens: + for _api, _token in apis_and_tokens: + token_suffix = f"...{_token[-4:]}" if _token else "unknown" if _api.rate_limiting[-1] == 60: self.logger.warning( - f"{self.log_prefix} API has rate limit set to 60 which indicates an invalid token, skipping" + f"{self.log_prefix} API has rate limit set to 60 which indicates an invalid token " + f"(token ending in '{token_suffix}'), skipping" ) continue - self.auto_verified_and_merged_users.append(_api.get_user().login) + try: + _api_user = _api.get_user().login + except GithubException as ex: + self.logger.warning( + f"{self.log_prefix} Failed to get API user for token ending in '{token_suffix}', skipping. {ex}" + ) + continue + + self.auto_verified_and_merged_users.append(_api_user) def prepare_log_prefix(self, pull_request: PullRequest | None = None) -> str: return prepare_log_prefix( From 2909797a07fafb2e72927b39a79ca1f11070851d Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 28 Jan 2026 08:12:42 +0200 Subject: [PATCH 2/6] update log level --- webhook_server/libs/github_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 2481906b..1287ca46 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -598,7 +598,7 @@ def add_api_users_to_auto_verified_and_merged_users(self) -> None: for _api, _token in apis_and_tokens: token_suffix = f"...{_token[-4:]}" if _token else "unknown" if _api.rate_limiting[-1] == 60: - self.logger.warning( + self.logger.error( f"{self.log_prefix} API has rate limit set to 60 which indicates an invalid token " f"(token ending in '{token_suffix}'), skipping" ) @@ -607,7 +607,7 @@ def add_api_users_to_auto_verified_and_merged_users(self) -> None: try: _api_user = _api.get_user().login except GithubException as ex: - self.logger.warning( + self.logger.error( f"{self.log_prefix} Failed to get API user for token ending in '{token_suffix}', skipping. {ex}" ) continue From 6cfb5168b36e3a2dfc98373910d8255d8cac069a Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 28 Jan 2026 19:39:29 +0200 Subject: [PATCH 3/6] update log level to exception --- webhook_server/libs/github_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 1287ca46..2c11ce1b 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -598,7 +598,7 @@ def add_api_users_to_auto_verified_and_merged_users(self) -> None: for _api, _token in apis_and_tokens: token_suffix = f"...{_token[-4:]}" if _token else "unknown" if _api.rate_limiting[-1] == 60: - self.logger.error( + self.logger.exception( f"{self.log_prefix} API has rate limit set to 60 which indicates an invalid token " f"(token ending in '{token_suffix}'), skipping" ) @@ -607,7 +607,7 @@ def add_api_users_to_auto_verified_and_merged_users(self) -> None: try: _api_user = _api.get_user().login except GithubException as ex: - self.logger.error( + self.logger.exception( f"{self.log_prefix} Failed to get API user for token ending in '{token_suffix}', skipping. {ex}" ) continue From f9136c01e7b928d4e608a2356dc0fd0e1ea397a6 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 28 Jan 2026 21:51:43 +0200 Subject: [PATCH 4/6] fix(github-api): add try/except for rate_limiting access Address CodeRabbit review comment: wrap _api.rate_limiting[-1] access in try/except to handle GithubException on invalid tokens. Also fix logger.exception to logger.warning for non-exception context. --- webhook_server/libs/github_api.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 2c11ce1b..df0f25eb 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -597,8 +597,17 @@ def add_api_users_to_auto_verified_and_merged_users(self) -> None: apis_and_tokens = get_apis_and_tokes_from_config(config=self.config) for _api, _token in apis_and_tokens: token_suffix = f"...{_token[-4:]}" if _token else "unknown" - if _api.rate_limiting[-1] == 60: - self.logger.exception( + try: + rate_limit_remaining = _api.rate_limiting[-1] + except GithubException as ex: + self.logger.warning( + f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', " + f"skipping. {ex}" + ) + continue + + if rate_limit_remaining == 60: + self.logger.warning( f"{self.log_prefix} API has rate limit set to 60 which indicates an invalid token " f"(token ending in '{token_suffix}'), skipping" ) From dccc59b1be685bd33c27b3851dd2ae7a264434a1 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 29 Jan 2026 12:23:20 +0200 Subject: [PATCH 5/6] fix(github-api): make add_api_users async with asyncio.to_thread Address CodeRabbit review: convert blocking PyGithub calls (_api.rate_limiting and _api.get_user()) to async using asyncio.to_thread() to prevent blocking the event loop. - Convert add_api_users_to_auto_verified_and_merged_users to async - Move call from __init__ to process() (async context) - Wrap blocking PyGithub calls with asyncio.to_thread() - Update test to handle async method --- webhook_server/libs/github_api.py | 13 ++++++++----- webhook_server/tests/test_github_api.py | 7 ++++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index df0f25eb..6880548f 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -178,8 +178,8 @@ def __init__(self, hook_data: dict[Any, Any], headers: Headers, logger: logging. # This prevents predictable paths and ensures isolation between concurrent webhook handlers self.clone_repo_dir: str = tempfile.mkdtemp(prefix=f"github-webhook-{self.repository_name}-") self._repo_cloned: bool = False # Track if repository has been cloned - # Initialize auto-verified users from API users - self.add_api_users_to_auto_verified_and_merged_users() + # Note: auto-verified users from API users are initialized in process() + # because the method is async and requires asyncio.to_thread() for blocking calls self.current_pull_request_supported_retest = self._current_pull_request_supported_retest self.issue_url_for_welcome_msg: str = ( @@ -408,6 +408,9 @@ def redact_output(value: str) -> str: raise RuntimeError(f"Repository clone failed: {ex}") from ex async def process(self) -> Any: + # Initialize auto-verified users from API users (async operation) + await self.add_api_users_to_auto_verified_and_merged_users() + event_log: str = f"Event type: {self.github_event}. event ID: {self.x_github_delivery}" # Start webhook routing context step @@ -593,12 +596,12 @@ async def process(self) -> Any: await self._update_context_metrics() return None - def add_api_users_to_auto_verified_and_merged_users(self) -> None: + async def add_api_users_to_auto_verified_and_merged_users(self) -> None: apis_and_tokens = get_apis_and_tokes_from_config(config=self.config) for _api, _token in apis_and_tokens: token_suffix = f"...{_token[-4:]}" if _token else "unknown" try: - rate_limit_remaining = _api.rate_limiting[-1] + rate_limit_remaining = await asyncio.to_thread(lambda api: api.rate_limiting[-1], _api) except GithubException as ex: self.logger.warning( f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', " @@ -614,7 +617,7 @@ def add_api_users_to_auto_verified_and_merged_users(self) -> None: continue try: - _api_user = _api.get_user().login + _api_user = await asyncio.to_thread(lambda api: api.get_user().login, _api) except GithubException as ex: self.logger.exception( f"{self.log_prefix} Failed to get API user for token ending in '{token_suffix}', skipping. {ex}" diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 5e0d58a9..7938655c 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -621,13 +621,14 @@ def test_init_failed_repository_objects( assert gh.repository is None assert not hasattr(gh, "repository_by_github_app") + @pytest.mark.asyncio @patch("webhook_server.libs.github_api.Config") @patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") @patch("webhook_server.libs.github_api.get_github_repo_api") @patch("webhook_server.libs.github_api.get_repository_github_app_api") @patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix") @patch("webhook_server.libs.github_api.get_apis_and_tokes_from_config") - def test_add_api_users_to_auto_verified_and_merged_users( + async def test_add_api_users_to_auto_verified_and_merged_users( self, mock_get_apis, mock_color, @@ -639,7 +640,7 @@ def test_add_api_users_to_auto_verified_and_merged_users( minimal_headers, logger, ) -> None: - """Test the add_api_users_to_auto_verified_and_merged_users property.""" + """Test the add_api_users_to_auto_verified_and_merged_users method.""" mock_config.return_value.repository = True mock_config.return_value.repository_local_data.return_value = {} mock_get_api.return_value = (Mock(), "token", "apiuser") @@ -689,7 +690,7 @@ def get_value_side_effect(value, *args, **kwargs): mock_api.get_user.return_value = mock_user mock_get_apis.return_value = [(mock_api, "token")] gh = GithubWebhook(minimal_hook_data, minimal_headers, logger) - _ = gh.add_api_users_to_auto_verified_and_merged_users + await gh.add_api_users_to_auto_verified_and_merged_users() assert "test-user" in gh.auto_verified_and_merged_users @patch("webhook_server.libs.github_api.get_apis_and_tokes_from_config") From fc690bb71d91b2a2257947f8b7b22e90d4b3f2b1 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 29 Jan 2026 13:55:35 +0200 Subject: [PATCH 6/6] refactor(github-api): parallelize token checks with asyncio.gather Address CodeRabbit review comments: - Refactor add_api_users_to_auto_verified_and_merged_users to process all tokens concurrently using asyncio.gather instead of sequential loop - Use TEST_GITHUB_TOKEN constant in test for consistency --- webhook_server/libs/github_api.py | 21 +++++++++++++-------- webhook_server/tests/test_github_api.py | 2 +- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 6880548f..e081758c 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -598,33 +598,38 @@ async def process(self) -> Any: async def add_api_users_to_auto_verified_and_merged_users(self) -> None: apis_and_tokens = get_apis_and_tokes_from_config(config=self.config) - for _api, _token in apis_and_tokens: - token_suffix = f"...{_token[-4:]}" if _token else "unknown" + + async def check_token(api: github.Github, token: str) -> str | None: + """Check a single API token and return the user login if valid, None otherwise.""" + token_suffix = f"...{token[-4:]}" if token else "unknown" try: - rate_limit_remaining = await asyncio.to_thread(lambda api: api.rate_limiting[-1], _api) + rate_limit_remaining = await asyncio.to_thread(lambda: api.rate_limiting[-1]) except GithubException as ex: self.logger.warning( f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', " f"skipping. {ex}" ) - continue + return None if rate_limit_remaining == 60: self.logger.warning( f"{self.log_prefix} API has rate limit set to 60 which indicates an invalid token " f"(token ending in '{token_suffix}'), skipping" ) - continue + return None try: - _api_user = await asyncio.to_thread(lambda api: api.get_user().login, _api) + _api_user = await asyncio.to_thread(lambda: api.get_user().login) except GithubException as ex: self.logger.exception( f"{self.log_prefix} Failed to get API user for token ending in '{token_suffix}', skipping. {ex}" ) - continue + return None + + return _api_user - self.auto_verified_and_merged_users.append(_api_user) + results = await asyncio.gather(*[check_token(api, token) for api, token in apis_and_tokens]) + self.auto_verified_and_merged_users.extend(user for user in results if user is not None) def prepare_log_prefix(self, pull_request: PullRequest | None = None) -> str: return prepare_log_prefix( diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 7938655c..3f8655a2 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -688,7 +688,7 @@ def get_value_side_effect(value, *args, **kwargs): mock_user = Mock() mock_user.login = "test-user" mock_api.get_user.return_value = mock_user - mock_get_apis.return_value = [(mock_api, "token")] + mock_get_apis.return_value = [(mock_api, TEST_GITHUB_TOKEN)] gh = GithubWebhook(minimal_hook_data, minimal_headers, logger) await gh.add_api_users_to_auto_verified_and_merged_users() assert "test-user" in gh.auto_verified_and_merged_users