From 7e22e2155f2abb3e97c76f27a4ea3d5367a29eb5 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 10 Nov 2025 15:12:36 +0200 Subject: [PATCH 1/3] fix: Prevent infinite loop in repository configuration on failures Fixed critical bug in get_future_results() where calling result.result() before checking for exceptions would crash the entire process when encountering failed repositories (e.g., archived repos). The crash would trigger auto-restart (Docker/systemd), creating an infinite crash-restart loop that prevented any repositories from being configured. Changes: - Check for exceptions using try-except BEFORE calling result.result() - Use logger.exception() for proper traceback capture - Continue processing remaining repositories on individual failures - Improved error message with actionable guidance Fixes: Archon task 3f6a599b-7d4d-4626-9b72-d367c525060e --- webhook_server/utils/helpers.py | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/webhook_server/utils/helpers.py b/webhook_server/utils/helpers.py index 1a72e7d1..d4e03bc4 100644 --- a/webhook_server/utils/helpers.py +++ b/webhook_server/utils/helpers.py @@ -470,19 +470,29 @@ def log_rate_limit(rate_limit: RateLimitOverview, api_user: str) -> None: def get_future_results(futures: list[Future]) -> None: """ - result must return tuple[bool, str, Callable] when the Callable is Logger function (LOGGER.info, LOGGER.error, etc) - """ - for result in as_completed(futures): - _res = result.result() - _log = _res[2] - if result.exception(): - _log(result.exception()) + Process futures from repository configuration tasks. - if _res[0]: - _log(_res[1]) + Args: + futures: List of futures that return (success, message, logger_func) tuples - else: + Notes: + Continues processing on exceptions to handle partial failures gracefully. + Worker threads may crash on archived repositories or API permission issues. + """ + logger = get_logger_with_params() + + for result in as_completed(futures): + try: + # CRITICAL FIX: Calling result.result() will raise exception if one exists + # This gives us proper exception context for logger.exception() + _res = result.result() + _log = _res[2] _log(_res[1]) + except Exception: + # Proper exception context - logger.exception() can capture traceback + logger.exception( + "Repository configuration crashed. Check for archived repositories or API permission issues." + ) def get_repository_color_for_log_prefix(repository_name: str, data_dir: str) -> str: From 68f723bad275dd38d78af4bf02df5d491a934494 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 10 Nov 2025 15:32:26 +0200 Subject: [PATCH 2/3] test: Fix exception path test and improve tuple unpacking in helpers - Fix DummyFutureException to properly raise exceptions in tests - Add assertions to verify exception handling works correctly - Improve tuple unpacking readability (success, message, logger_func) Addresses CodeRabbit review comments on fix/repository-settings-infinite-loop --- webhook_server/tests/test_helpers.py | 25 ++++++++++++++++++++----- webhook_server/utils/helpers.py | 5 ++--- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/webhook_server/tests/test_helpers.py b/webhook_server/tests/test_helpers.py index 8252b36a..73965411 100644 --- a/webhook_server/tests/test_helpers.py +++ b/webhook_server/tests/test_helpers.py @@ -316,18 +316,33 @@ def exception(self): def log(self, msg): self.logged = msg - # Exception result + # Exception result - result() should RAISE the exception class DummyFutureException: def result(self): - return (False, "fail", lambda msg: self.log(msg)) + raise RuntimeError("Repository configuration crashed") def exception(self): - return Exception("fail-exc") + return RuntimeError("Repository configuration crashed") def log(self, msg): self.logged = msg futures = [DummyFuture(), DummyFutureFail(), DummyFutureException()] - # Patch as_completed to just yield the futures + + # Patch as_completed to just yield the futures and capture logger calls with patch("webhook_server.utils.helpers.as_completed", return_value=futures): - get_future_results(futures) + with patch("webhook_server.utils.helpers.get_logger_with_params") as mock_get_logger: + mock_logger = Mock() + mock_get_logger.return_value = mock_logger + + get_future_results(futures) + + # Verify logger.exception was called for the exception case + mock_logger.exception.assert_called_once_with( + "Repository configuration crashed. Check for archived repositories or API permission issues." + ) + + # Verify all futures were processed (success and failure futures should have logged) + assert futures[0].logged == "success" + assert futures[1].logged == "fail" + # futures[2] raised exception so no log attribute set diff --git a/webhook_server/utils/helpers.py b/webhook_server/utils/helpers.py index d4e03bc4..dc18e793 100644 --- a/webhook_server/utils/helpers.py +++ b/webhook_server/utils/helpers.py @@ -485,9 +485,8 @@ def get_future_results(futures: list[Future]) -> None: try: # CRITICAL FIX: Calling result.result() will raise exception if one exists # This gives us proper exception context for logger.exception() - _res = result.result() - _log = _res[2] - _log(_res[1]) + success, message, logger_func = result.result() + logger_func(message) except Exception: # Proper exception context - logger.exception() can capture traceback logger.exception( From a03a47be27758ddb2ec467b755fec812a3a3e099 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 10 Nov 2025 15:51:14 +0200 Subject: [PATCH 3/3] fix: Replace unused unpacked variables with underscore (RUF059) Fixed all 36 RUF059 linting errors by replacing unused variables in tuple unpacking with '_' placeholder. This improves code clarity by explicitly marking values that are intentionally ignored. Changes: - Updated test files to use '_' for unused tuple values - Enabled RUF059 rule in pyproject.toml to prevent future violations - Improved helper functions to discard unused return values - Applied .get() pattern in edge case validation Affected files: - pyproject.toml: Added RUF059 to linting rules - test_helpers_sanitization.py: 2 fixes (stderr unused) - test_runner_handler.py: 20 fixes (rc/out/err unused) - test_webhook.py: 10 fixes (message/log_func unused) - test_edge_cases_validation.py: 2 fixes (dict.get pattern) - helpers.py: 1 fix (success unused) - app_utils.py: 1 fix (str() to \!s conversion) --- pyproject.toml | 2 +- .../tests/test_edge_cases_validation.py | 4 ++-- .../tests/test_helpers_sanitization.py | 4 ++-- webhook_server/tests/test_runner_handler.py | 24 +++++++++---------- webhook_server/tests/test_webhook.py | 20 ++++++++-------- webhook_server/utils/app_utils.py | 2 +- webhook_server/utils/helpers.py | 2 +- 7 files changed, 29 insertions(+), 29 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 0fc6a546..2d95cd5b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -15,7 +15,7 @@ fix = true output-format = "grouped" [tool.ruff.lint] -select = ["E", "F", "W", "I", "B", "UP", "PLC0415", "ARG"] +select = ["E", "F", "W", "I", "B", "UP", "PLC0415", "ARG", "RUF059"] [tool.ruff.lint.per-file-ignores] "webhook_server/tests/*" = ["ARG"] diff --git a/webhook_server/tests/test_edge_cases_validation.py b/webhook_server/tests/test_edge_cases_validation.py index d694918f..ef500377 100644 --- a/webhook_server/tests/test_edge_cases_validation.py +++ b/webhook_server/tests/test_edge_cases_validation.py @@ -548,9 +548,9 @@ def test_multiple_filter_combinations(self): assert isinstance(filtered, list) # Verify all filter conditions are satisfied for entry in filtered: - if "level" in filter_params and filter_params["level"]: + if filter_params.get("level"): assert entry.level == filter_params["level"] - if "repository" in filter_params and filter_params["repository"]: + if filter_params.get("repository"): assert entry.repository == filter_params["repository"] diff --git a/webhook_server/tests/test_helpers_sanitization.py b/webhook_server/tests/test_helpers_sanitization.py index f297e26c..b8dc6fe4 100644 --- a/webhook_server/tests/test_helpers_sanitization.py +++ b/webhook_server/tests/test_helpers_sanitization.py @@ -379,7 +379,7 @@ async def test_run_command_mask_sensitive_false(self) -> None: mock_get_logger.return_value = mock_logger # Run command with a secret and mask_sensitive=False - success, stdout, stderr = await run_command( + success, stdout, _ = await run_command( command="echo 'token: ghp_test123'", log_prefix="test", redact_secrets=["ghp_test123"], @@ -411,7 +411,7 @@ async def test_run_command_mask_sensitive_true(self) -> None: mock_get_logger.return_value = mock_logger # Run command with a secret and mask_sensitive=True - success, stdout, stderr = await run_command( + success, stdout, _ = await run_command( command="echo 'token: ghp_test456'", log_prefix="test", redact_secrets=["ghp_test456"], diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 1bbb8365..fe0134bc 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -99,7 +99,7 @@ async def test_run_podman_command_success(self, runner_handler: RunnerHandler) - with patch( "webhook_server.libs.handlers.runner_handler.run_command", new=AsyncMock(return_value=(True, "success", "")) ): - rc, out, err = await runner_handler.run_podman_command("podman build .") + rc, out, _ = await runner_handler.run_podman_command("podman build .") assert rc is True assert "success" in out # Relaxed assertion @@ -110,7 +110,7 @@ async def test_run_podman_command_podman_bug(self, runner_handler: RunnerHandler with patch("webhook_server.libs.handlers.runner_handler.run_command", new=AsyncMock()) as mock_run: mock_run.side_effect = [(False, "output", podman_bug_err), (True, "success after fix", "")] with patch.object(runner_handler, "fix_podman_bug") as mock_fix: - rc, out, err = await runner_handler.run_podman_command("podman build .") + _, _, _ = await runner_handler.run_podman_command("podman build .") assert mock_fix.call_count >= 1 @pytest.mark.asyncio @@ -120,7 +120,7 @@ async def test_run_podman_command_other_error(self, runner_handler: RunnerHandle "webhook_server.libs.handlers.runner_handler.run_command", new=AsyncMock(return_value=(False, "output", "other error")), ): - rc, out, err = await runner_handler.run_podman_command("podman build .") + rc, _, _ = await runner_handler.run_podman_command("podman build .") assert rc is False or rc is None @pytest.mark.asyncio @@ -510,7 +510,7 @@ async def test_prepare_cloned_repo_dir_success( async with runner_handler._prepare_cloned_repo_dir( "/tmp/test-repo-unique", mock_pull_request ) as result: - success, out, err = result + success, _, _ = result assert success is True @pytest.mark.asyncio @@ -521,7 +521,7 @@ async def test_prepare_cloned_repo_dir_clone_failure(self, runner_handler: Runne new=AsyncMock(return_value=(False, "output", "error")), ): async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-unique2") as result: - success, out, err = result + success, out, _ = result assert success is False assert out == "output" @@ -536,7 +536,7 @@ async def test_prepare_cloned_repo_dir_with_checkout( async with runner_handler._prepare_cloned_repo_dir( "/tmp/test-repo-unique3", mock_pull_request, checkout="feature-branch" ) as result: - success, out, err = result + success, _, _ = result assert success is True @pytest.mark.asyncio @@ -550,7 +550,7 @@ async def test_prepare_cloned_repo_dir_with_tag( async with runner_handler._prepare_cloned_repo_dir( "/tmp/test-repo-unique4", mock_pull_request, tag_name="v1.0.0" ) as result: - success, out, err = result + success, _, _ = result assert success is True @pytest.mark.asyncio @@ -564,7 +564,7 @@ async def test_prepare_cloned_repo_dir_merged_pr( async with runner_handler._prepare_cloned_repo_dir( "/tmp/test-repo-unique5", mock_pull_request, is_merged=True ) as result: - success, out, err = result + success, _, _ = result assert success is True @pytest.mark.asyncio @@ -583,7 +583,7 @@ async def run_command_side_effect(*args, **kwargs): new=AsyncMock(side_effect=run_command_side_effect), ): async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-x", mock_pull_request) as result: - success, out, err = result + success, out, _ = result assert not success assert out == "fail" @@ -605,7 +605,7 @@ async def run_command_side_effect(*args, **kwargs): new=AsyncMock(side_effect=run_command_side_effect), ): async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-x", mock_pull_request) as result: - success, out, err = result + success, out, _ = result assert not success assert out == "fail" @@ -627,7 +627,7 @@ async def run_command_side_effect(*args, **kwargs): new=AsyncMock(side_effect=run_command_side_effect), ): async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-x", mock_pull_request) as result: - success, out, err = result + success, out, _ = result assert not success assert out == "fail" @@ -653,7 +653,7 @@ async def run_command_side_effect(*args, **kwargs): new=AsyncMock(side_effect=run_command_side_effect), ): async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-x", mock_pull_request) as result: - success, out, err = result + success, out, _ = result assert not success assert out == "fail" diff --git a/webhook_server/tests/test_webhook.py b/webhook_server/tests/test_webhook.py index 727350bb..24564359 100644 --- a/webhook_server/tests/test_webhook.py +++ b/webhook_server/tests/test_webhook.py @@ -42,7 +42,7 @@ def test_process_github_webhook_success_no_existing_hooks( """Test successful webhook creation when no existing hooks exist.""" mock_get_repo_api.return_value = mock_repo - success, message, log_func = process_github_webhook( + success, message, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict ) @@ -70,7 +70,7 @@ def test_process_github_webhook_success_with_secret( """Test successful webhook creation with secret.""" mock_get_repo_api.return_value = mock_repo - success, message, log_func = process_github_webhook( + success, _, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", @@ -100,7 +100,7 @@ def test_process_github_webhook_default_events( mock_get_repo_api.return_value = mock_repo data_without_events = {"name": "owner/test-repo"} - success, message, log_func = process_github_webhook( + success, _, _ = process_github_webhook( repository_name="test-repo", data=data_without_events, webhook_ip="http://example.com", apis_dict=apis_dict ) @@ -129,7 +129,7 @@ def test_process_github_webhook_existing_hook_same_config( mock_repo.get_hooks.return_value = [existing_hook] mock_get_repo_api.return_value = mock_repo - success, message, log_func = process_github_webhook( + success, message, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict ) @@ -160,7 +160,7 @@ def test_process_github_webhook_secret_mismatch_deletes_old_hook( mock_repo.get_hooks.return_value = [existing_hook] mock_get_repo_api.return_value = mock_repo - success, message, log_func = process_github_webhook( + success, _, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", @@ -204,7 +204,7 @@ def test_process_github_webhook_secret_removal_deletes_old_hook( mock_repo.get_hooks.return_value = [existing_hook] mock_get_repo_api.return_value = mock_repo - success, message, log_func = process_github_webhook( + success, _, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", @@ -234,7 +234,7 @@ def test_process_github_webhook_missing_api(self, sample_data: dict[str, Any]) - } } - success, message, log_func = process_github_webhook( + success, message, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict_no_api ) @@ -249,7 +249,7 @@ def test_process_github_webhook_repository_not_found( """Test error handling when repository cannot be found.""" mock_get_repo_api.return_value = None - success, message, log_func = process_github_webhook( + success, message, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict ) @@ -270,7 +270,7 @@ def test_process_github_webhook_hooks_listing_error( mock_repo.get_hooks.side_effect = Exception("Permission denied") mock_get_repo_api.return_value = mock_repo - success, message, log_func = process_github_webhook( + success, message, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict ) @@ -299,7 +299,7 @@ def test_process_github_webhook_multiple_existing_hooks( mock_repo.get_hooks.return_value = [matching_hook, non_matching_hook] mock_get_repo_api.return_value = mock_repo - success, message, log_func = process_github_webhook( + success, message, _ = process_github_webhook( repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict ) diff --git a/webhook_server/utils/app_utils.py b/webhook_server/utils/app_utils.py index 3c63fbf7..121e4dc5 100644 --- a/webhook_server/utils/app_utils.py +++ b/webhook_server/utils/app_utils.py @@ -115,5 +115,5 @@ def parse_datetime_string(datetime_str: str | None, field_name: str) -> datetime except ValueError as e: raise HTTPException( status_code=400, - detail=f"Invalid {field_name} format: {datetime_str}. Expected ISO 8601 format. Error: {str(e)}", + detail=f"Invalid {field_name} format: {datetime_str}. Expected ISO 8601 format. Error: {e!s}", ) from e diff --git a/webhook_server/utils/helpers.py b/webhook_server/utils/helpers.py index dc18e793..e2f8a62b 100644 --- a/webhook_server/utils/helpers.py +++ b/webhook_server/utils/helpers.py @@ -485,7 +485,7 @@ def get_future_results(futures: list[Future]) -> None: try: # CRITICAL FIX: Calling result.result() will raise exception if one exists # This gives us proper exception context for logger.exception() - success, message, logger_func = result.result() + _, message, logger_func = result.result() logger_func(message) except Exception: # Proper exception context - logger.exception() can capture traceback