diff --git a/CLAUDE.md b/CLAUDE.md index 312cb150f..978156fac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -71,7 +71,7 @@ python3 scripts/aws/filter_log_events_across_streams.py --hours 12 --owner Foxqu ## Coding Standards - **No DOCSTRINGS**: Don't add unless told. Don't delete existing unless outdated. -- **COMMENTS**: Don't delete unless outdated. Preserve URLs. One line when possible. +- **COMMENTS**: Don't delete unless outdated. Preserve URLs. One line when possible. **Don't hard-wrap sentences mid-thought** — if a comment is two sentences, two lines is fine; but don't break one sentence across multiple lines because it makes the comment unreadable when scanned. Let the editor wrap it visually. - **LOGGERS**: Every `continue`, `break`, `return` inside a function MUST have a preceding `logger.info(...)` (or warning/error). Also log at every conditional branch to show which path was taken. - **`set_xxx` EARLIEST**: Call `set_trigger`, `set_owner_repo`, `set_pr_number` etc. at the earliest point in each handler, right after the value is known. - **Don't repeat structured log context**: `set_owner_repo` in `main.py` already adds `owner_repo` to every log entry. Don't repeat owner/repo in individual logger messages. diff --git a/pyproject.toml b/pyproject.toml index 5775dc2de..4442f7fec 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "GitAuto" -version = "1.27.0" +version = "1.33.0" requires-python = ">=3.14" dependencies = [ "annotated-doc==0.0.4", diff --git a/scripts/lint/check_cast_usage.sh b/scripts/lint/check_cast_usage.sh index d05422deb..0634071f9 100755 --- a/scripts/lint/check_cast_usage.sh +++ b/scripts/lint/check_cast_usage.sh @@ -2,13 +2,15 @@ # Detect `cast` usage from typing module in staged Python implementation files. # Rule: No cast in impl files — fix underlying types or use isinstance narrowing. # Allowed in test files (test_*.py, conftest.py) for TypedDict fixtures. +# Also allowed in utils/error/handle_exceptions.py: the generic decorator pattern with ParamSpec + TypeVar genuinely can't round-trip types without cast (async wrapper's Coroutine[...R] vs declared Callable[P, R]; helper returns typed Any). set -uo pipefail STAGED_PY_FILES=$(git diff --cached --name-only --diff-filter=d -- '*.py' \ | grep -v '^\.\?venv/' \ | grep -v '^schemas/' \ | grep -vE '(^|/)test_' \ - | grep -vE '(^|/)conftest\.py$') + | grep -vE '(^|/)conftest\.py$' \ + | grep -v '^utils/error/handle_exceptions\.py$') if [ -z "$STAGED_PY_FILES" ]; then exit 0 diff --git a/services/aws/test_delete_scheduler.py b/services/aws/test_delete_scheduler.py index 1babfcb8b..c43100602 100644 --- a/services/aws/test_delete_scheduler.py +++ b/services/aws/test_delete_scheduler.py @@ -204,15 +204,17 @@ def test_delete_scheduler_with_various_schedule_names( def test_delete_scheduler_logging_level(mock_scheduler_client): - """Test that the function uses logger.info for success messages.""" + """Test that the function uses logger.info for success messages. Patch the + full module-level logger (not logger.info directly) so handle_exceptions' + shared Logger.info calls hit the real logger, not this test's mock.""" schedule_name = "test-schedule" mock_scheduler_client.delete_schedule.return_value = None - with patch("services.aws.delete_scheduler.logger.info") as mock_info: + with patch("services.aws.delete_scheduler.logger") as mock_logger: result = delete_scheduler(schedule_name) assert result is True - mock_info.assert_called_once_with( + mock_logger.info.assert_called_once_with( "Deleted EventBridge Scheduler: %s", schedule_name ) diff --git a/services/git/git_commit_and_push.py b/services/git/git_commit_and_push.py index 1b8602aab..b6174d5d4 100644 --- a/services/git/git_commit_and_push.py +++ b/services/git/git_commit_and_push.py @@ -21,6 +21,7 @@ def git_commit_and_push( run_subprocess(["git", "add"] + files, clone_dir) if skip_ci: + logger.info("git_commit_and_push: appending [skip ci] to message") message = f"{message} [skip ci]" message = format_commit_message(message=message, base_args=base_args) @@ -31,6 +32,7 @@ def git_commit_and_push( run_subprocess(["git", "remote", "set-url", "origin", clone_url], clone_dir) push_cmd = ["git", "push"] if force: + logger.info("git_commit_and_push: force push via --force-with-lease") push_cmd.append("--force-with-lease") push_cmd.extend(["origin", f"HEAD:refs/heads/{new_branch}"]) run_subprocess(push_cmd, clone_dir) diff --git a/services/git/test_git_commit_and_push.py b/services/git/test_git_commit_and_push.py index a99936096..d1295cc88 100644 --- a/services/git/test_git_commit_and_push.py +++ b/services/git/test_git_commit_and_push.py @@ -8,6 +8,7 @@ from services.git.git_clone_to_tmp import git_clone_to_tmp from services.git.git_commit_and_push import git_commit_and_push +from utils.error.handle_exceptions import TRANSIENT_MAX_ATTEMPTS def test_git_commit_and_push_success(create_test_base_args): @@ -89,8 +90,11 @@ def mock_run(args, cwd): files=["file.py"], ) assert result is True - # -m is at index 2, message is at index 3 - assert "[skip ci]" in commit_args_captured[3] + # commit_args_captured[3] is the full message: " [skip ci]" + # followed by a "\n\nCo-Authored-By: ..." trailer appended by + # format_commit_message. Assert the subject line (first line) exactly. + assert commit_args_captured[:3] == ["git", "commit", "-m"] + assert commit_args_captured[3].splitlines()[0] == "Update file.py [skip ci]" def test_git_commit_and_push_stages_specific_files(create_test_base_args): @@ -109,8 +113,7 @@ def mock_run(args, cwd): files=["old.py", "new.py"], ) assert result is True - assert "old.py" in add_args_captured - assert "new.py" in add_args_captured + assert add_args_captured == ["git", "add", "old.py", "new.py"] def test_git_commit_and_push_force_push(create_test_base_args): @@ -122,15 +125,26 @@ def mock_run(args, cwd): push_args_captured = args return MagicMock(returncode=0, stdout="") + base_args = create_test_base_args( + clone_url="https://x-access-token:tok@github.com/o/r.git", + new_branch="feature/force-push", + ) + with patch("services.git.git_commit_and_push.run_subprocess", side_effect=mock_run): result = git_commit_and_push( - base_args=create_test_base_args(), + base_args=base_args, message="Rebase onto release/20260422", files=["app.py"], force=True, ) assert result is True - assert "--force-with-lease" in push_args_captured + assert push_args_captured == [ + "git", + "push", + "--force-with-lease", + "origin", + "HEAD:refs/heads/feature/force-push", + ] def test_git_commit_and_push_no_force_by_default(create_test_base_args): @@ -142,14 +156,104 @@ def mock_run(args, cwd): push_args_captured = args return MagicMock(returncode=0, stdout="") + base_args = create_test_base_args( + clone_url="https://x-access-token:tok@github.com/o/r.git", + new_branch="feature/normal-push", + ) + with patch("services.git.git_commit_and_push.run_subprocess", side_effect=mock_run): result = git_commit_and_push( - base_args=create_test_base_args(), + base_args=base_args, message="Normal push", files=["app.py"], ) assert result is True - assert "--force-with-lease" not in push_args_captured + assert push_args_captured == [ + "git", + "push", + "origin", + "HEAD:refs/heads/feature/normal-push", + ] + + +def test_git_commit_and_push_retries_on_github_500(create_test_base_args): + """Sentry AGENT-36Z/36J: GitHub returns transient 500 on push. Retry and succeed.""" + attempts = {"push": 0} + + def mock_run(args, cwd): + if args[:2] == ["git", "push"]: + attempts["push"] += 1 + if attempts["push"] == 1: + raise ValueError( + "Command failed: remote: Internal Server Error\n" + "To https://github.com/org/repo.git\n" + " ! [remote rejected] HEAD -> branch (Internal Server Error)" + ) + return MagicMock(returncode=0, stdout="") + + with ( + patch("services.git.git_commit_and_push.run_subprocess", side_effect=mock_run), + patch("utils.error.handle_exceptions.time.sleep"), + ): + result = git_commit_and_push( + base_args=create_test_base_args(), + message="Update file.py", + files=["file.py"], + ) + assert result is True + assert attempts["push"] == 2 + + +def test_git_commit_and_push_gives_up_after_max_github_500s(create_test_base_args): + """If GitHub keeps 500-ing past the retry budget, surface the failure.""" + attempts = {"push": 0} + + def mock_run(args, cwd): + if args[:2] == ["git", "push"]: + attempts["push"] += 1 + raise ValueError( + "Command failed: remote: Internal Server Error\n" + " ! [remote rejected] HEAD -> branch (Internal Server Error)" + ) + return MagicMock(returncode=0, stdout="") + + with ( + patch("services.git.git_commit_and_push.run_subprocess", side_effect=mock_run), + patch("utils.error.handle_exceptions.time.sleep"), + ): + result = git_commit_and_push( + base_args=create_test_base_args(), + message="Update file.py", + files=["file.py"], + ) + assert result is False + assert attempts["push"] == TRANSIENT_MAX_ATTEMPTS + + +def test_git_commit_and_push_does_not_retry_non_transient(create_test_base_args): + """A 'pathspec did not match' or similar non-transient failure must NOT retry — + otherwise we'd waste time on errors that will never succeed.""" + attempts = {"push": 0} + + def mock_run(args, cwd): + if args[:2] == ["git", "push"]: + attempts["push"] += 1 + raise ValueError( + "Command failed: error: src refspec main does not match any" + ) + return MagicMock(returncode=0, stdout="") + + with ( + patch("services.git.git_commit_and_push.run_subprocess", side_effect=mock_run), + patch("utils.error.handle_exceptions.time.sleep"), + ): + result = git_commit_and_push( + base_args=create_test_base_args(), + message="Update file.py", + files=["file.py"], + ) + assert result is False + assert attempts["push"] == 1 @pytest.mark.integration @@ -179,10 +283,12 @@ def test_git_commit_and_push_to_local_bare(local_repo, create_test_base_args): bare_dir = bare_url.replace("file://", "") log = subprocess.run( - ["git", "log", "--oneline", "feature/sociable-push", "-1"], + ["git", "log", "--format=%s", "feature/sociable-push", "-1"], cwd=bare_dir, capture_output=True, text=True, check=False, ) - assert "Add new file" in log.stdout + # Commit message has a trailer appended by format_commit_message; assert + # the subject line (first line) matches exactly. + assert log.stdout.strip().splitlines()[0] == "Add new file" diff --git a/services/github/files/test_get_remote_file_content_by_url.py b/services/github/files/test_get_remote_file_content_by_url.py index 1117f3475..1ec1b03ac 100644 --- a/services/github/files/test_get_remote_file_content_by_url.py +++ b/services/github/files/test_get_remote_file_content_by_url.py @@ -450,7 +450,16 @@ def test_different_branch_and_commit_sha( file_path, content = get_remote_file_content_by_url(url, "test-token") assert file_path == "src/test.py" - assert "```src/test.py" in content + assert content == ( + "```src/test.py\n" + "1: def hello_world():\n" + "2: print('Hello, World!')\n" + "3: return 'success'\n" + "4: \n" + "5: if __name__ == '__main__':\n" + "6: hello_world()\n" + "```" + ) mock_requests_get.assert_called_once_with( url="https://api.github.com/repos/test-owner/test-repo/contents/src/test.py?ref=feature-branch", headers={"Authorization": "Bearer test-token"}, @@ -483,7 +492,16 @@ def test_nested_file_path( file_path, content = get_remote_file_content_by_url(url, "test-token") assert file_path == "src/utils/helpers/file.py" - assert "```src/utils/helpers/file.py" in content + assert content == ( + "```src/utils/helpers/file.py\n" + "1: def hello_world():\n" + "2: print('Hello, World!')\n" + "3: return 'success'\n" + "4: \n" + "5: if __name__ == '__main__':\n" + "6: hello_world()\n" + "```" + ) mock_requests_get.assert_called_once_with( url="https://api.github.com/repos/test-owner/test-repo/contents/src/utils/helpers/file.py?ref=main", headers={"Authorization": "Bearer test-token"}, @@ -629,7 +647,17 @@ def test_various_parameter_combinations( result_file_path, content = get_remote_file_content_by_url(url, "test-token") assert result_file_path == file_path - assert f"```{file_path}" in content + # Exact content: fenced block with filename header and numbered sample. + assert content == ( + f"```{file_path}\n" + "1: def hello_world():\n" + "2: print('Hello, World!')\n" + "3: return 'success'\n" + "4: \n" + "5: if __name__ == '__main__':\n" + "6: hello_world()\n" + "```" + ) expected_url = f"https://api.github.com/repos/{owner}/{repo}/contents/{file_path}?ref={ref}" mock_requests_get.assert_called_once_with( url=expected_url, @@ -670,11 +698,13 @@ def test_large_file_content( file_path, content = get_remote_file_content_by_url(url, "test-token") assert file_path == "src/test.py" - assert "```src/test.py" in content - assert "1: # Large file" in content - assert "101: print('line')" in content - # Should have 102 lines total (1 header + 100 print lines + 1 empty line at end) - assert len(content.split("\n")) >= 104 # Including the header lines + # Build the exact expected content: "# Large file" + 100 "print('line')" + # rows + one trailing empty line (trailing newline in `large_content`). + numbered = ["1: # Large file"] + numbered.extend(f"{i + 2}: print('line')" for i in range(100)) + numbered.append("102: ") # trailing empty line from large_content's final \n + expected_body = "\n".join(numbered) + assert content == f"```src/test.py\n{expected_body}\n```" @patch("services.github.files.get_remote_file_content_by_url.requests.get") @patch("services.github.files.get_remote_file_content_by_url.create_headers") @@ -694,9 +724,7 @@ def test_404_returns_empty_without_sentry( mock_response.status_code = 404 mock_requests_get.return_value = mock_response - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: url = "https://github.com/test-owner/test-repo/blob/main/missing.py" result = get_remote_file_content_by_url(url, "test-token") diff --git a/utils/error/handle_exceptions.py b/utils/error/handle_exceptions.py index 952d087b3..51dc8edac 100644 --- a/utils/error/handle_exceptions.py +++ b/utils/error/handle_exceptions.py @@ -10,16 +10,21 @@ # Third party imports import requests -import sentry_sdk -from utils.error.is_billing_error import is_billing_error -from utils.error.is_server_error import is_server_error +from utils.error.handle_generic_error import handle_generic_error +from utils.error.handle_http_error import handle_http_error +from utils.error.handle_json_error import handle_json_error +from utils.error.is_transient_error import is_transient_error from utils.logging.logging_config import logger P = ParamSpec("P") # Function parameters (args, kwargs) R = TypeVar("R") # Return type of decorated function D = TypeVar("D") # Default return value type +# Retry policy for transient-error retries. 5xx responses almost always mean the server did not complete the operation, so retry is safe by default. Kept small so non-retryable bugs still surface quickly in Sentry. +TRANSIENT_MAX_ATTEMPTS = 3 +TRANSIENT_BACKOFF_SECONDS = 2 + @overload def handle_exceptions( @@ -53,153 +58,140 @@ def handle_exceptions( ) -> Callable[[Callable[P, R]], Callable[P, R]]: ... -def _handle_http_error( - err: requests.HTTPError, - func_name: str, - log_args: list, - log_kwargs: dict, - api_type: str, - raise_on_error: bool, - error_return: Any, - retry_callback: Callable[[], Any], -): - if err.response is None: - if raise_on_error: - raise err - return error_return, False - - status_code: int = err.response.status_code - - if is_server_error(err): - logger.warning( - "%s received server error %s, not reporting to Sentry", - func_name, - status_code, - ) - if raise_on_error: - raise err - return error_return, False - - reason: str | Any = ( - str(err.response.reason) if err.response.reason is not None else "Unknown" - ) - text: str | Any = ( - str(err.response.text) if err.response.text is not None else "Unknown" - ) - logger.error("reason: %s, text: %s, status_code: %s", reason, text, status_code) - - if api_type == "github" and status_code in {403, 429}: - logger.error("err.response.headers: %s", err.response.headers) - limit = int(err.response.headers["X-RateLimit-Limit"]) - remaining = int(err.response.headers["X-RateLimit-Remaining"]) - used = int(err.response.headers["X-RateLimit-Used"]) - - if remaining == 0: - reset_ts = int(err.response.headers.get("X-RateLimit-Reset", 0)) - current_ts = int(time.time()) - wait_time = reset_ts - current_ts - if wait_time <= 0: - wait_time = 1 - else: - wait_time = wait_time + 5 - err_msg = f"{func_name} encountered a GitHubPrimaryRateLimitError: {err}. Retrying after {wait_time} seconds. Limit: {limit}, Remaining: {remaining}, Used: {used}. Reason: {reason}. Text: {text}" - logger.error(err_msg) - time.sleep(wait_time) - return retry_callback(), True - - if "exceeded a secondary rate limit" in err.response.text.lower(): - retry_after = int(err.response.headers.get("Retry-After", 60)) - err_msg = f"{func_name} encountered a GitHubSecondaryRateLimitError: {err}. Retrying after {retry_after} seconds. Limit: {limit}, Remaining: {remaining}, Used: {used}. Reason: {reason}. Text: {text}" - logger.error(err_msg) - time.sleep(retry_after) - return retry_callback(), True - - err_msg = f"{func_name} encountered an HTTPError: {err}. Limit: {limit}, Remaining: {remaining}, Used: {used}. Reason: {reason}. Text: {text}" - sentry_sdk.capture_exception(err) - logger.error(err_msg) - if raise_on_error: - raise err - - elif api_type == "web_search" and status_code == 429: - err_msg = f"Web Search Rate Limit in {func_name}()" - logger.error(err_msg) - logger.error("err.response.headers: %s", err.response.headers) - raise err - - else: - err_msg = f"{func_name} encountered an HTTPError: {err}\n\nArgs: {json.dumps(log_args, indent=2, default=str)}\n\nKwargs: {json.dumps(log_kwargs, indent=2, default=str)}\n\nReason: {reason}\n\nText: {text}" - sentry_sdk.capture_exception(err) - logger.error(err_msg) - - if raise_on_error: - raise err - return error_return, False - - -def _handle_json_error( - err: json.JSONDecodeError, - func_name: str, - log_args: list, - log_kwargs: dict, - raise_on_error: bool, - error_return: Any, -): - raw_response = err.doc if hasattr(err, "doc") else "Raw response not available" - err_msg = f"{func_name} encountered a JSONDecodeError: {err}\n\nRaw response: {raw_response}\n\nArgs: {json.dumps(log_args, indent=2, default=str)}\n\nKwargs: {json.dumps(log_kwargs, indent=2, default=str)}" - sentry_sdk.capture_exception(err) - logger.error(err_msg) - if raise_on_error: - raise err - return error_return - - -def _handle_generic_error( - err: Exception, - func_name: str, - log_args: list, - log_kwargs: dict, - raise_on_error: bool, - error_return: Any, -): - err_msg = f"{func_name} encountered an {type(err).__name__}: {err}\n\nArgs: {json.dumps(log_args, indent=2, default=str)}\n\nKwargs: {json.dumps(log_kwargs, indent=2, default=str)}" - if is_server_error(err): - logger.warning("%s received server error, not reporting to Sentry", func_name) - logger.warning(err_msg) - elif is_billing_error(err): - logger.warning("%s received billing error, not reporting to Sentry", func_name) - logger.warning(err_msg) - else: - sentry_sdk.capture_exception(err) - logger.error(err_msg) - if raise_on_error: - raise err - return error_return - - def handle_exceptions( default_return_value: Any = None, raise_on_error: bool = False, api_type: str = "github", ) -> Callable[[Callable[P, R]], Callable[P, R]]: - """https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#checking-the-status-of-your-rate-limit""" + """https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#checking-the-status-of-your-rate-limit + + Transient upstream failures (GitHub 5xx, remote Internal Server Error from subprocess, etc., per is_transient_error) are retried up to TRANSIENT_MAX_ATTEMPTS times with linear backoff. 5xx responses almost always mean the server did not complete the operation, so retry is safe by default for all decorated functions. + """ def decorator(func: Callable[P, R]) -> Callable[P, R]: if inspect.iscoroutinefunction(func): + logger.info("handle_exceptions: wrapping async %s", func.__name__) @wraps(wrapped=func) async def async_wrapper(*args: P.args, **kwargs: P.kwargs) -> R: log_args = list(args) log_kwargs = dict(kwargs) - if callable(default_return_value): + logger.info( + "%s computing default_return_value callable", func.__name__ + ) error_return = default_return_value(*args, **kwargs) else: + logger.info("%s using static default_return_value", func.__name__) error_return = default_return_value + remaining_transient_retries = TRANSIENT_MAX_ATTEMPTS - 1 + attempt = 0 + while True: + attempt += 1 + try: + logger.info("%s invoking attempt %d", func.__name__, attempt) + return await func(*args, **kwargs) + except requests.HTTPError as err: + result, retried = handle_http_error( + err, + func.__name__, + log_args, + log_kwargs, + api_type, + raise_on_error, + error_return, + lambda: async_wrapper(*args, **kwargs), + ) + if retried: + logger.info( + "%s HTTPError awaiting retry result", func.__name__ + ) + return await result + logger.info("%s HTTPError returning non-retry", func.__name__) + return cast(R, result) + except json.JSONDecodeError as err: + logger.info( + "%s hit JSONDecodeError, handing off", func.__name__ + ) + return cast( + R, + handle_json_error( + err, + func.__name__, + log_args, + log_kwargs, + raise_on_error, + error_return, + ), + ) + except asyncio.CancelledError: + logger.warning( + "%s was cancelled (CancelledError)", func.__name__ + ) + if raise_on_error: + logger.error("%s re-raising CancelledError", func.__name__) + raise + logger.info( + "%s returning default for CancelledError", func.__name__ + ) + return cast(R, error_return) + except Exception as err: + if remaining_transient_retries > 0 and is_transient_error(err): + logger.info( + "%s transient-error branch taken", func.__name__ + ) + backoff = TRANSIENT_BACKOFF_SECONDS * attempt + logger.warning( + "%s transient failure on attempt %d, retrying in %ds: %s", + func.__name__, + attempt, + backoff, + err, + ) + remaining_transient_retries -= 1 + await asyncio.sleep(backoff) + logger.info("%s retrying after backoff", func.__name__) + continue + logger.info( + "%s generic error (non-retry), handing off", func.__name__ + ) + return cast( + R, + handle_generic_error( + err, + func.__name__, + log_args, + log_kwargs, + raise_on_error, + error_return, + ), + ) + + logger.info( + "handle_exceptions: returning async wrapper for %s", func.__name__ + ) + return cast(Callable[P, R], async_wrapper) + logger.info("handle_exceptions: wrapping sync %s", func.__name__) + @wraps(wrapped=func) + def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: + log_args = list(args) + log_kwargs = dict(kwargs) + if callable(default_return_value): + logger.info("%s computing default_return_value callable", func.__name__) + error_return = default_return_value(*args, **kwargs) + else: + logger.info("%s using static default_return_value", func.__name__) + error_return = default_return_value + remaining_transient_retries = TRANSIENT_MAX_ATTEMPTS - 1 + attempt = 0 + while True: + attempt += 1 try: - return await func(*args, **kwargs) + logger.info("%s invoking attempt %d", func.__name__, attempt) + return func(*args, **kwargs) except requests.HTTPError as err: - result, retried = _handle_http_error( + result, retried = handle_http_error( err, func.__name__, log_args, @@ -207,15 +199,18 @@ async def async_wrapper(*args: P.args, **kwargs: P.kwargs) -> R: api_type, raise_on_error, error_return, - lambda: async_wrapper(*args, **kwargs), + lambda: wrapper(*args, **kwargs), ) if retried: - return await result + logger.info("%s HTTPError retry result ready", func.__name__) + return cast(R, result) + logger.info("%s HTTPError returning non-retry", func.__name__) return cast(R, result) except json.JSONDecodeError as err: + logger.info("%s hit JSONDecodeError, handing off", func.__name__) return cast( R, - _handle_json_error( + handle_json_error( err, func.__name__, log_args, @@ -224,15 +219,27 @@ async def async_wrapper(*args: P.args, **kwargs: P.kwargs) -> R: error_return, ), ) - except asyncio.CancelledError: - logger.warning("%s was cancelled (CancelledError)", func.__name__) - if raise_on_error: - raise - return cast(R, error_return) except Exception as err: + if remaining_transient_retries > 0 and is_transient_error(err): + logger.info("%s transient-error branch taken", func.__name__) + backoff = TRANSIENT_BACKOFF_SECONDS * attempt + logger.warning( + "%s transient failure on attempt %d, retrying in %ds: %s", + func.__name__, + attempt, + backoff, + err, + ) + remaining_transient_retries -= 1 + time.sleep(backoff) + logger.info("%s retrying after backoff", func.__name__) + continue + logger.info( + "%s generic error (non-retry), handing off", func.__name__ + ) return cast( R, - _handle_generic_error( + handle_generic_error( err, func.__name__, log_args, @@ -242,59 +249,8 @@ async def async_wrapper(*args: P.args, **kwargs: P.kwargs) -> R: ), ) - return cast(Callable[P, R], async_wrapper) - - @wraps(wrapped=func) - def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: - log_args = list(args) - log_kwargs = dict(kwargs) - - if callable(default_return_value): - error_return = default_return_value(*args, **kwargs) - else: - error_return = default_return_value - - try: - return func(*args, **kwargs) - except requests.HTTPError as err: - result, retried = _handle_http_error( - err, - func.__name__, - log_args, - log_kwargs, - api_type, - raise_on_error, - error_return, - lambda: wrapper(*args, **kwargs), - ) - if retried: - return cast(R, result) - return cast(R, result) - except json.JSONDecodeError as err: - return cast( - R, - _handle_json_error( - err, - func.__name__, - log_args, - log_kwargs, - raise_on_error, - error_return, - ), - ) - except Exception as err: - return cast( - R, - _handle_generic_error( - err, - func.__name__, - log_args, - log_kwargs, - raise_on_error, - error_return, - ), - ) - + logger.info("handle_exceptions: returning sync wrapper for %s", func.__name__) return wrapper + logger.info("handle_exceptions: returning decorator") return decorator diff --git a/utils/error/handle_generic_error.py b/utils/error/handle_generic_error.py new file mode 100644 index 000000000..eb652f5c7 --- /dev/null +++ b/utils/error/handle_generic_error.py @@ -0,0 +1,37 @@ +import json +from typing import Any + +import sentry_sdk + +from utils.error.is_billing_error import is_billing_error +from utils.error.is_server_error import is_server_error +from utils.logging.logging_config import logger + + +def handle_generic_error( + err: Exception, + func_name: str, + log_args: list, + log_kwargs: dict, + raise_on_error: bool, + error_return: Any, +): + err_msg = f"{func_name} encountered an {type(err).__name__}: {err}\n\nArgs: {json.dumps(log_args, indent=2, default=str)}\n\nKwargs: {json.dumps(log_kwargs, indent=2, default=str)}" + + if is_server_error(err): + logger.warning("%s received server error, not reporting to Sentry", func_name) + logger.warning(err_msg) + elif is_billing_error(err): + logger.warning("%s received billing error, not reporting to Sentry", func_name) + logger.warning(err_msg) + else: + logger.info("%s reporting generic error to Sentry", func_name) + sentry_sdk.capture_exception(err) + logger.error(err_msg) + + if raise_on_error: + logger.error("%s generic error re-raising", func_name) + raise err + + logger.info("%s generic error returning default", func_name) + return error_return diff --git a/utils/error/handle_github_rate_limit.py b/utils/error/handle_github_rate_limit.py new file mode 100644 index 000000000..1d4ccc5f7 --- /dev/null +++ b/utils/error/handle_github_rate_limit.py @@ -0,0 +1,65 @@ +import time +from typing import Any, Callable + +import requests +import sentry_sdk + +from utils.logging.logging_config import logger + + +def handle_github_rate_limit( + err: requests.HTTPError, + func_name: str, + reason: str, + text: str, + raise_on_error: bool, + retry_callback: Callable[[], Any], +): + """Handle GitHub's 403/429 responses for primary and secondary rate limits. + + Returns (retry_result, True) when a retry was performed; returns None when + the caller should fall through to generic Sentry-capture-and-return + behaviour (non-rate-limit 403/429, or raise_on_error already raised).""" + assert err.response is not None + logger.error("err.response.headers: %s", err.response.headers) + limit = int(err.response.headers["X-RateLimit-Limit"]) + remaining = int(err.response.headers["X-RateLimit-Remaining"]) + used = int(err.response.headers["X-RateLimit-Used"]) + + if remaining == 0: + logger.info("%s GitHub primary rate limit exhausted", func_name) + reset_ts = int(err.response.headers.get("X-RateLimit-Reset", 0)) + current_ts = int(time.time()) + wait_time = reset_ts - current_ts + + if wait_time <= 0: + logger.info("%s rate-limit reset already past, waiting 1s", func_name) + wait_time = 1 + else: + logger.info("%s waiting %ds for rate-limit reset", func_name, wait_time) + wait_time = wait_time + 5 + + err_msg = f"{func_name} encountered a GitHubPrimaryRateLimitError: {err}. Retrying after {wait_time} seconds. Limit: {limit}, Remaining: {remaining}, Used: {used}. Reason: {reason}. Text: {text}" + logger.error(err_msg) + time.sleep(wait_time) + logger.info("%s invoking primary-rate-limit retry", func_name) + return retry_callback(), True + + if "exceeded a secondary rate limit" in err.response.text.lower(): + logger.info("%s GitHub secondary rate limit hit", func_name) + retry_after = int(err.response.headers.get("Retry-After", 60)) + err_msg = f"{func_name} encountered a GitHubSecondaryRateLimitError: {err}. Retrying after {retry_after} seconds. Limit: {limit}, Remaining: {remaining}, Used: {used}. Reason: {reason}. Text: {text}" + logger.error(err_msg) + time.sleep(retry_after) + logger.info("%s invoking secondary-rate-limit retry", func_name) + return retry_callback(), True + + err_msg = f"{func_name} encountered an HTTPError: {err}. Limit: {limit}, Remaining: {remaining}, Used: {used}. Reason: {reason}. Text: {text}" + sentry_sdk.capture_exception(err) + logger.error(err_msg) + if raise_on_error: + logger.error("%s re-raising 403/429 HTTPError", func_name) + raise err + + logger.info("%s 403/429 non-rate-limit, falling through to default", func_name) + return None diff --git a/utils/error/handle_http_error.py b/utils/error/handle_http_error.py new file mode 100644 index 000000000..b9c66eff5 --- /dev/null +++ b/utils/error/handle_http_error.py @@ -0,0 +1,81 @@ +import json +from typing import Any, Callable + +import requests +import sentry_sdk + +from utils.error.handle_github_rate_limit import handle_github_rate_limit +from utils.error.is_server_error import is_server_error +from utils.logging.logging_config import logger + + +def handle_http_error( + err: requests.HTTPError, + func_name: str, + log_args: list, + log_kwargs: dict, + api_type: str, + raise_on_error: bool, + error_return: Any, + retry_callback: Callable[[], Any], +): + if err.response is None: + logger.info("%s HTTPError has no response object", func_name) + if raise_on_error: + logger.error("%s HTTPError with no response, re-raising", func_name) + raise err + + logger.info("%s HTTPError with no response, returning default", func_name) + return error_return, False + + status_code: int = err.response.status_code + + if is_server_error(err): + logger.warning( + "%s received server error %s, not reporting to Sentry", + func_name, + status_code, + ) + if raise_on_error: + logger.error("%s re-raising server error %s", func_name, status_code) + raise err + + logger.info("%s returning default for server error %s", func_name, status_code) + return error_return, False + + reason: str | Any = ( + str(err.response.reason) if err.response.reason is not None else "Unknown" + ) + text: str | Any = ( + str(err.response.text) if err.response.text is not None else "Unknown" + ) + logger.error("reason: %s, text: %s, status_code: %s", reason, text, status_code) + + if api_type == "github" and status_code in {403, 429}: + logger.info("%s dispatching to github rate-limit handler", func_name) + retry_result = handle_github_rate_limit( + err, func_name, reason, text, raise_on_error, retry_callback + ) + if retry_result is not None: + logger.info("%s github 403/429 returned retry result", func_name) + return retry_result + + elif api_type == "web_search" and status_code == 429: + logger.info("%s web_search hit 429, raising", func_name) + err_msg = f"Web Search Rate Limit in {func_name}()" + logger.error(err_msg) + logger.error("err.response.headers: %s", err.response.headers) + raise err + + else: + logger.info("%s reporting HTTPError to Sentry", func_name) + err_msg = f"{func_name} encountered an HTTPError: {err}\n\nArgs: {json.dumps(log_args, indent=2, default=str)}\n\nKwargs: {json.dumps(log_kwargs, indent=2, default=str)}\n\nReason: {reason}\n\nText: {text}" + sentry_sdk.capture_exception(err) + logger.error(err_msg) + + if raise_on_error: + logger.error("%s HTTPError path re-raising", func_name) + raise err + + logger.info("%s HTTPError path returning default", func_name) + return error_return, False diff --git a/utils/error/handle_json_error.py b/utils/error/handle_json_error.py new file mode 100644 index 000000000..c9386aa0a --- /dev/null +++ b/utils/error/handle_json_error.py @@ -0,0 +1,26 @@ +import json +from typing import Any + +import sentry_sdk + +from utils.logging.logging_config import logger + + +def handle_json_error( + err: json.JSONDecodeError, + func_name: str, + log_args: list, + log_kwargs: dict, + raise_on_error: bool, + error_return: Any, +): + raw_response = err.doc if hasattr(err, "doc") else "Raw response not available" + err_msg = f"{func_name} encountered a JSONDecodeError: {err}\n\nRaw response: {raw_response}\n\nArgs: {json.dumps(log_args, indent=2, default=str)}\n\nKwargs: {json.dumps(log_kwargs, indent=2, default=str)}" + sentry_sdk.capture_exception(err) + logger.error(err_msg) + if raise_on_error: + logger.error("%s JSONDecodeError re-raising", func_name) + raise err + + logger.info("%s JSONDecodeError returning default", func_name) + return error_return diff --git a/utils/error/is_transient_error.py b/utils/error/is_transient_error.py new file mode 100644 index 000000000..2eb2c907c --- /dev/null +++ b/utils/error/is_transient_error.py @@ -0,0 +1,31 @@ +from utils.error.is_server_error import is_server_error +from utils.logging.logging_config import logger + +# Marker strings that indicate a transient GitHub-side failure surfaced through +# non-HTTPError exceptions (e.g. ValueError from run_subprocess wrapping `git +# push` stderr). Matched case-sensitive to avoid false positives on user text. +TRANSIENT_MARKERS = ( + "remote: Internal Server Error", + "502 Bad Gateway", + "503 Service Unavailable", + "504 Gateway Timeout", + "HTTP 500", + "HTTP 502", + "HTTP 503", + "HTTP 504", +) + + +def is_transient_error(err: Exception): + """Return True when the exception represents a transient upstream failure + worth retrying. Covers both HTTP-shaped errors (via is_server_error) and + string-reported errors from subprocesses that the agent's retry path can + safely try again.""" + if is_server_error(err): + logger.info("is_transient_error: matched via is_server_error") + return True + + err_text = str(err) + matched = any(marker in err_text for marker in TRANSIENT_MARKERS) + logger.info("is_transient_error: marker match=%s", matched) + return matched diff --git a/utils/error/test_handle_exceptions.py b/utils/error/test_handle_exceptions.py index 38dc05604..c7b8f35f2 100644 --- a/utils/error/test_handle_exceptions.py +++ b/utils/error/test_handle_exceptions.py @@ -5,7 +5,7 @@ import pytest import requests -from utils.error.handle_exceptions import handle_exceptions +from utils.error.handle_exceptions import handle_exceptions, TRANSIENT_MAX_ATTEMPTS @handle_exceptions(default_return_value=None, raise_on_error=False) @@ -269,9 +269,7 @@ def test_handle_exceptions_http_error_no_retry(): """Test that non-rate-limit HTTP errors don't trigger retry and report to sentry.""" with patch("utils.error.test_handle_exceptions.requests.get") as mock_get, patch( "utils.error.handle_exceptions.time.sleep" - ) as mock_sleep, patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + ) as mock_sleep, patch("sentry_sdk.capture_exception") as mock_sentry: mock_response = MagicMock() mock_response.status_code = 404 @@ -295,7 +293,7 @@ def test_handle_exceptions_http_error_no_retry(): def test_handle_exceptions_connection_error(): """Test that connection errors are handled without retry and report to sentry.""" with patch("utils.error.test_handle_exceptions.requests.get") as mock_get, patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" + "sentry_sdk.capture_exception" ) as mock_sentry: conn_error = requests.exceptions.ConnectionError("Connection failed") mock_get.side_effect = conn_error @@ -310,7 +308,7 @@ def test_handle_exceptions_connection_error(): def test_handle_exceptions_timeout_error(): """Test that timeout errors are handled without retry and report to sentry.""" with patch("utils.error.test_handle_exceptions.requests.get") as mock_get, patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" + "sentry_sdk.capture_exception" ) as mock_sentry: timeout_error = requests.exceptions.Timeout("Request timed out") mock_get.side_effect = timeout_error @@ -325,7 +323,7 @@ def test_handle_exceptions_timeout_error(): def test_handle_exceptions_json_decode_error(): """Test that JSON decode errors are handled without retry and report to sentry.""" with patch("utils.error.test_handle_exceptions.requests.get") as mock_get, patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" + "sentry_sdk.capture_exception" ) as mock_sentry: mock_response = MagicMock() json_error = requests.exceptions.JSONDecodeError("Invalid JSON", "", 0) @@ -343,7 +341,7 @@ def test_handle_exceptions_json_decode_error(): def test_handle_exceptions_generic_exception(): """Test that generic exceptions are handled and report to sentry.""" with patch("utils.error.test_handle_exceptions.requests.get") as mock_get, patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" + "sentry_sdk.capture_exception" ) as mock_sentry: value_error = ValueError("Generic error") mock_get.side_effect = value_error @@ -432,9 +430,7 @@ def func_raises_502(): error.response = response raise error - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: result = func_raises_502() assert result == "fallback" mock_sentry.assert_not_called() @@ -451,9 +447,7 @@ def func_raises_503(): error.response = response raise error - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: result = func_raises_503() assert result == "fallback" mock_sentry.assert_not_called() @@ -469,9 +463,7 @@ class FakeServerError(Exception): def func_raises_anthropic_500(): raise FakeServerError("Internal server error") - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: result = func_raises_anthropic_500() assert result == "fallback" mock_sentry.assert_not_called() @@ -486,9 +478,7 @@ def func_raises_supabase_502(): err.code = "502" # type: ignore[attr-defined] raise err - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: result = func_raises_supabase_502() assert result == "fallback" mock_sentry.assert_not_called() @@ -503,9 +493,7 @@ def func_raises_supabase_404(): err.code = "PGRST204" # type: ignore[attr-defined] raise err - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: result = func_raises_supabase_404() assert result == "fallback" mock_sentry.assert_called_once() @@ -559,9 +547,7 @@ def func_403_remaining(): error.response = response raise error - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: with pytest.raises(requests.exceptions.HTTPError): func_403_remaining() mock_sentry.assert_called_once() @@ -596,9 +582,7 @@ def func_raises_404(): error.response = response raise error - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: with pytest.raises(requests.exceptions.HTTPError): func_raises_404() mock_sentry.assert_called_once() @@ -612,7 +596,7 @@ def func_json_error_no_doc(): delattr(error, "doc") raise error - with patch("utils.error.handle_exceptions.sentry_sdk.capture_exception"): + with patch("sentry_sdk.capture_exception"): result = func_json_error_no_doc() assert result == "default" @@ -622,9 +606,7 @@ def test_handle_exceptions_json_decode_error_raise_on_error(): def func_json_error_raises(): raise json.JSONDecodeError("Invalid", "bad json", 0) - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: with pytest.raises(json.JSONDecodeError): func_json_error_raises() mock_sentry.assert_called_once() @@ -632,7 +614,7 @@ def func_json_error_raises(): @pytest.mark.asyncio async def test_async_handle_exceptions_returns_none_on_error(): - with patch("utils.error.handle_exceptions.sentry_sdk.capture_exception"): + with patch("sentry_sdk.capture_exception"): coro = async_mock_function_for_testing() assert coro is not None result = await coro @@ -641,7 +623,7 @@ async def test_async_handle_exceptions_returns_none_on_error(): @pytest.mark.asyncio async def test_async_handle_exceptions_returns_custom_default(): - with patch("utils.error.handle_exceptions.sentry_sdk.capture_exception"): + with patch("sentry_sdk.capture_exception"): coro = async_mock_function_with_custom_default() assert coro is not None result = await coro @@ -650,7 +632,7 @@ async def test_async_handle_exceptions_returns_custom_default(): @pytest.mark.asyncio async def test_async_handle_exceptions_raises_when_raise_on_error_true(): - with patch("utils.error.handle_exceptions.sentry_sdk.capture_exception"): + with patch("sentry_sdk.capture_exception"): with pytest.raises(ValueError, match="Async test error"): coro = async_mock_function_with_raise_on_error() assert coro is not None @@ -667,9 +649,7 @@ async def test_async_handle_exceptions_success_case(): @pytest.mark.asyncio async def test_async_handle_exceptions_reports_to_sentry(): - with patch( - "utils.error.handle_exceptions.sentry_sdk.capture_exception" - ) as mock_sentry: + with patch("sentry_sdk.capture_exception") as mock_sentry: coro = async_mock_function_for_testing() assert coro is not None await coro @@ -694,3 +674,53 @@ async def func_gets_cancelled(): with pytest.raises(asyncio.CancelledError): await func_gets_cancelled() + + +# --- Transient retry behavior (AGENT-36Z/36J) --- + + +def test_decorator_retries_on_transient_value_error(): + """Sentry AGENT-36Z/36J: retry a ValueError whose message indicates a + transient upstream failure, then succeed on the second attempt.""" + attempts = {"count": 0} + + @handle_exceptions(default_return_value=None, raise_on_error=False) + def flaky(): + attempts["count"] += 1 + if attempts["count"] == 1: + raise ValueError("Command failed: remote: Internal Server Error") + return "ok" + + with patch("utils.error.handle_exceptions.time.sleep"): + assert flaky() == "ok" + assert attempts["count"] == 2 + + +def test_decorator_gives_up_after_max_attempts_on_transient_error(): + """If every attempt hits a transient error, surface the failure via the + default_return_value — the bounded retry prevents infinite loops.""" + attempts = {"count": 0} + + @handle_exceptions(default_return_value="DEFAULT", raise_on_error=False) + def always_500(): + attempts["count"] += 1 + raise ValueError("Command failed: HTTP 502 Bad Gateway") + + with patch("utils.error.handle_exceptions.time.sleep"): + assert always_500() == "DEFAULT" + # TRANSIENT_MAX_ATTEMPTS controls the total number of attempts. + assert attempts["count"] == TRANSIENT_MAX_ATTEMPTS + + +def test_decorator_does_not_retry_non_transient_error(): + """Non-transient failures must fail fast on the first attempt.""" + attempts = {"count": 0} + + @handle_exceptions(default_return_value="DEFAULT", raise_on_error=False) + def real_bug(): + attempts["count"] += 1 + raise ValueError("Command failed: fatal: pathspec 'x' did not match any files") + + with patch("utils.error.handle_exceptions.time.sleep"): + assert real_bug() == "DEFAULT" + assert attempts["count"] == 1 diff --git a/utils/error/test_handle_generic_error.py b/utils/error/test_handle_generic_error.py new file mode 100644 index 000000000..bf07450d6 --- /dev/null +++ b/utils/error/test_handle_generic_error.py @@ -0,0 +1,61 @@ +from unittest.mock import patch + +import pytest +import requests + +from utils.error.handle_generic_error import handle_generic_error + + +def test_non_special_error_captures_sentry_and_returns_default(): + err = ValueError("some bug") + + with patch("utils.error.handle_generic_error.sentry_sdk") as mock_sentry: + result = handle_generic_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + raise_on_error=False, + error_return="DEFAULT", + ) + + assert result == "DEFAULT" + mock_sentry.capture_exception.assert_called_once_with(err) + + +def test_server_error_does_not_report_to_sentry(): + """is_server_error covers HTTPError/SDK-shaped 5xx — those should suppress Sentry.""" + response = requests.Response() + response.status_code = 500 + err = requests.HTTPError() + err.response = response + + with patch("utils.error.handle_generic_error.sentry_sdk") as mock_sentry: + result = handle_generic_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + raise_on_error=False, + error_return="DEFAULT", + ) + + assert result == "DEFAULT" + mock_sentry.capture_exception.assert_not_called() + + +def test_raise_on_error_re_raises_original_exception(): + err = RuntimeError("boom") + + with patch("utils.error.handle_generic_error.sentry_sdk"): + with pytest.raises(RuntimeError) as excinfo: + handle_generic_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + raise_on_error=True, + error_return="DEFAULT", + ) + + assert excinfo.value is err diff --git a/utils/error/test_handle_github_rate_limit.py b/utils/error/test_handle_github_rate_limit.py new file mode 100644 index 000000000..e52513c33 --- /dev/null +++ b/utils/error/test_handle_github_rate_limit.py @@ -0,0 +1,111 @@ +from unittest.mock import Mock, patch + +import requests + +from utils.error.handle_github_rate_limit import handle_github_rate_limit + + +def _github_http_error( + status_code: int, + limit: int, + remaining: int, + used: int, + reset_ts: int = 0, + retry_after: int = 60, + body: str = "", +): + response = requests.Response() + response.status_code = status_code + response.headers["X-RateLimit-Limit"] = str(limit) + response.headers["X-RateLimit-Remaining"] = str(remaining) + response.headers["X-RateLimit-Used"] = str(used) + response.headers["X-RateLimit-Reset"] = str(reset_ts) + response.headers["Retry-After"] = str(retry_after) + response._content = body.encode("utf-8") # pylint: disable=protected-access + response.reason = "Forbidden" if status_code == 403 else "Too Many Requests" + err = requests.HTTPError() + err.response = response + return err + + +def test_primary_rate_limit_retries_via_callback(): + err = _github_http_error(403, limit=5000, remaining=0, used=5000) + retry_callback = Mock(return_value="RETRIED") + + with patch("utils.error.handle_github_rate_limit.time.sleep"): + result = handle_github_rate_limit( + err, + func_name="test_func", + reason="Forbidden", + text="rate limit exceeded", + raise_on_error=False, + retry_callback=retry_callback, + ) + + assert result == ("RETRIED", True) + retry_callback.assert_called_once_with() + + +def test_secondary_rate_limit_retries_via_callback(): + err = _github_http_error( + 403, + limit=5000, + remaining=100, + used=4900, + body="You have exceeded a secondary rate limit", + ) + retry_callback = Mock(return_value="RETRIED") + + with patch("utils.error.handle_github_rate_limit.time.sleep"): + result = handle_github_rate_limit( + err, + func_name="test_func", + reason="Forbidden", + text="exceeded a secondary rate limit", + raise_on_error=False, + retry_callback=retry_callback, + ) + + assert result == ("RETRIED", True) + retry_callback.assert_called_once_with() + + +def test_non_rate_limit_403_returns_none_for_fall_through(): + """403 without rate-limit signatures should fall through to the caller's + generic Sentry-capture path.""" + err = _github_http_error( + 403, limit=5000, remaining=100, used=4900, body="Forbidden" + ) + + with patch("utils.error.handle_github_rate_limit.sentry_sdk"): + result = handle_github_rate_limit( + err, + func_name="test_func", + reason="Forbidden", + text="not rate limited", + raise_on_error=False, + retry_callback=Mock(), + ) + + assert result is None + + +def test_non_rate_limit_403_re_raises_when_raise_on_error_true(): + err = _github_http_error( + 403, limit=5000, remaining=100, used=4900, body="Forbidden" + ) + + with patch("utils.error.handle_github_rate_limit.sentry_sdk"): + try: + handle_github_rate_limit( + err, + func_name="test_func", + reason="Forbidden", + text="not rate limited", + raise_on_error=True, + retry_callback=Mock(), + ) + except requests.HTTPError as raised: + assert raised is err + else: + raise AssertionError("expected HTTPError to be re-raised") diff --git a/utils/error/test_handle_http_error.py b/utils/error/test_handle_http_error.py new file mode 100644 index 000000000..40082252f --- /dev/null +++ b/utils/error/test_handle_http_error.py @@ -0,0 +1,116 @@ +from unittest.mock import Mock, patch + +import requests + +from utils.error.handle_http_error import handle_http_error + + +def _http_error(status_code: int, body: str = "error", reason: str = "Reason"): + response = requests.Response() + response.status_code = status_code + response.reason = reason + response._content = body.encode("utf-8") # pylint: disable=protected-access + err = requests.HTTPError() + err.response = response + return err + + +def test_no_response_returns_default(): + err = requests.HTTPError() + err.response = None + + result = handle_http_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + api_type="github", + raise_on_error=False, + error_return="DEFAULT", + retry_callback=Mock(), + ) + + assert result == ("DEFAULT", False) + + +def test_no_response_raises_when_raise_on_error_true(): + err = requests.HTTPError() + err.response = None + + try: + handle_http_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + api_type="github", + raise_on_error=True, + error_return="DEFAULT", + retry_callback=Mock(), + ) + except requests.HTTPError as raised: + assert raised is err + else: + raise AssertionError("expected HTTPError to be re-raised") + + +def test_server_error_returns_default_without_sentry(): + err = _http_error(500, body="internal", reason="Internal Server Error") + + with patch("utils.error.handle_http_error.sentry_sdk") as mock_sentry: + result = handle_http_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + api_type="github", + raise_on_error=False, + error_return="DEFAULT", + retry_callback=Mock(), + ) + + assert result == ("DEFAULT", False) + mock_sentry.capture_exception.assert_not_called() + + +def test_github_rate_limit_is_delegated_and_retries(): + err = _http_error(403, body="rate limit exceeded", reason="Forbidden") + err.response.headers["X-RateLimit-Limit"] = "5000" + err.response.headers["X-RateLimit-Remaining"] = "0" + err.response.headers["X-RateLimit-Used"] = "5000" + err.response.headers["X-RateLimit-Reset"] = "0" + retry_callback = Mock(return_value="RETRIED") + + with patch("utils.error.handle_github_rate_limit.time.sleep"): + result = handle_http_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + api_type="github", + raise_on_error=False, + error_return="DEFAULT", + retry_callback=retry_callback, + ) + + assert result == ("RETRIED", True) + retry_callback.assert_called_once_with() + + +def test_other_http_error_captures_sentry_and_returns_default(): + err = _http_error(418, body="teapot", reason="I'm a teapot") + + with patch("utils.error.handle_http_error.sentry_sdk") as mock_sentry: + result = handle_http_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + api_type="github", + raise_on_error=False, + error_return="DEFAULT", + retry_callback=Mock(), + ) + + assert result == ("DEFAULT", False) + mock_sentry.capture_exception.assert_called_once_with(err) diff --git a/utils/error/test_handle_json_error.py b/utils/error/test_handle_json_error.py new file mode 100644 index 000000000..6310fbc17 --- /dev/null +++ b/utils/error/test_handle_json_error.py @@ -0,0 +1,48 @@ +import json +from unittest.mock import patch + +import pytest + +from utils.error.handle_json_error import handle_json_error + + +def _make_json_error(): + try: + json.loads("not json") + except json.JSONDecodeError as err: + return err + raise AssertionError("json.loads unexpectedly succeeded") + + +def test_returns_default_and_reports_to_sentry(): + err = _make_json_error() + + with patch("utils.error.handle_json_error.sentry_sdk") as mock_sentry: + result = handle_json_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + raise_on_error=False, + error_return="DEFAULT", + ) + + assert result == "DEFAULT" + mock_sentry.capture_exception.assert_called_once_with(err) + + +def test_re_raises_when_raise_on_error_true(): + err = _make_json_error() + + with patch("utils.error.handle_json_error.sentry_sdk"): + with pytest.raises(json.JSONDecodeError) as excinfo: + handle_json_error( + err, + func_name="test", + log_args=[], + log_kwargs={}, + raise_on_error=True, + error_return="DEFAULT", + ) + + assert excinfo.value is err diff --git a/utils/error/test_is_transient_error.py b/utils/error/test_is_transient_error.py new file mode 100644 index 000000000..0455aa54a --- /dev/null +++ b/utils/error/test_is_transient_error.py @@ -0,0 +1,73 @@ +import requests + +from utils.error.is_transient_error import is_transient_error + + +def _http_error_with_status(status_code: int): + response = requests.Response() + response.status_code = status_code + err = requests.HTTPError() + err.response = response + return err + + +def test_http_500_is_transient(): + assert is_transient_error(_http_error_with_status(500)) is True + + +def test_http_502_is_transient(): + assert is_transient_error(_http_error_with_status(502)) is True + + +def test_http_503_is_transient(): + assert is_transient_error(_http_error_with_status(503)) is True + + +def test_http_400_is_not_transient(): + assert is_transient_error(_http_error_with_status(400)) is False + + +def test_http_404_is_not_transient(): + assert is_transient_error(_http_error_with_status(404)) is False + + +def test_valueerror_with_remote_internal_server_error_is_transient(): + err = ValueError( + "Command failed: remote: Internal Server Error\n" + "To https://github.com/org/repo.git\n" + " ! [remote rejected] HEAD -> branch (Internal Server Error)" + ) + assert is_transient_error(err) is True + + +def test_valueerror_with_502_bad_gateway_is_transient(): + assert is_transient_error(ValueError("Command failed: 502 Bad Gateway")) is True + + +def test_valueerror_with_503_service_unavailable_is_transient(): + assert ( + is_transient_error(ValueError("Command failed: 503 Service Unavailable")) + is True + ) + + +def test_valueerror_with_504_gateway_timeout_is_transient(): + assert is_transient_error(ValueError("Command failed: 504 Gateway Timeout")) is True + + +def test_valueerror_with_pathspec_error_is_not_transient(): + """A bad pathspec is a real bug — must NOT retry, or we'd burn time on + errors that can never succeed.""" + err = ValueError( + "Command failed: fatal: pathspec 'mongodb-binaries/x.tgz' did not match any files" + ) + assert is_transient_error(err) is False + + +def test_valueerror_with_non_fast_forward_is_not_transient(): + err = ValueError("Command failed: ! [rejected] HEAD -> branch (fetch first)") + assert is_transient_error(err) is False + + +def test_generic_runtimeerror_is_not_transient(): + assert is_transient_error(RuntimeError("something else broke")) is False diff --git a/uv.lock b/uv.lock index a8a66c967..707b59cde 100644 --- a/uv.lock +++ b/uv.lock @@ -596,7 +596,7 @@ wheels = [ [[package]] name = "gitauto" -version = "1.27.0" +version = "1.33.0" source = { virtual = "." } dependencies = [ { name = "annotated-doc" },