diff --git a/backend/app/services/intelligence/github/api_client.py b/backend/app/services/intelligence/github/api_client.py index e56e9030..4adc2310 100644 --- a/backend/app/services/intelligence/github/api_client.py +++ b/backend/app/services/intelligence/github/api_client.py @@ -90,22 +90,11 @@ async def fetch_repos_raw( ) if resp.status_code == 404: raise GitHubUserNotFoundError(username) + # レート制限(403 + 残量 0 / 429)は共通ヘルパで RetryableError に変換する + _raise_if_rate_limited(resp) if resp.status_code == 403: - # GitHub は rate limit でも 403 を返すため、ヘッダで判別する - if _is_rate_limited(resp): - retry_after = _retry_after_from_github(resp) - logger.warning( - "GitHub API rate limit hit (retry_after=%s)", retry_after, - ) - raise RetryableError( - "GitHub API rate limit", retry_after=retry_after, - ) + # レート制限でない 403 は権限エラー等の恒久障害 raise NonRetryableError(f"GitHub API 403 Forbidden: {resp.text[:200]}") - if resp.status_code == 429: - retry_after = _retry_after_from_github(resp) - raise RetryableError( - "GitHub API 429 Too Many Requests", retry_after=retry_after, - ) if resp.status_code in _RETRYABLE_STATUS_CODES: raise RetryableError(f"GitHub API {resp.status_code}") if 400 <= resp.status_code < 500: @@ -121,9 +110,35 @@ async def fetch_repos_raw( def _is_rate_limited(response: httpx.Response) -> bool: - """GitHub のレスポンスがレート制限起因かを判定する。""" - remaining = response.headers.get("x-ratelimit-remaining") - return remaining == "0" + """GitHub のレスポンスがレート制限起因かを判定する。 + + GitHub は API レート制限を超過すると **403** を返す(残量ヘッダで判別する)。 + 二次的な abuse 検出などで **429** を返すこともあり、こちらは残量に依らずレート制限扱い。 + """ + if response.status_code == 429: + return True + if response.status_code == 403: + return response.headers.get("x-ratelimit-remaining") == "0" + return False + + +def _raise_if_rate_limited(response: httpx.Response) -> None: + """レスポンスがレート制限(403 + 残量 0 / 429)なら ``RetryableError`` を raise する。 + + GitHub の全 fetch 経路で共通のレート制限ハンドリング。レート制限は特定リポの問題では + なくトークン単位のグローバルなスロットリングのため、``([], partial)`` として黙って + 握り込むと後続リポも同様に空収集になり、証跡が静かに欠ける。``retry_after`` 付きで + raise し直し、リセット窓を待ってタスク全体を再試行させる(``fetch_repos_raw`` と同方針)。 + レート制限でなければ何もしない(呼び出し側が他ステータスを処理する)。 + """ + if _is_rate_limited(response): + retry_after = _retry_after_from_github(response) + logger.warning( + "GitHub API rate limit hit (status=%s, retry_after=%s)", + response.status_code, + retry_after, + ) + raise RetryableError("GitHub API rate limit", retry_after=retry_after) def _retry_after_from_github(response: httpx.Response) -> float | None: @@ -155,8 +170,11 @@ async def fetch_languages( return {} try: resp = await client.get(f"/repos/{owner}/{repo}/languages") + # レート制限は握り込まず RetryableError で連携全体をリトライさせる(#485) + _raise_if_rate_limited(resp) if resp.status_code == 403: - logger.warning("Rate limit on languages for %s/%s", owner, repo) + # レート制限でない 403(ブロック等)は言語情報を欠いたまま best-effort 継続 + logger.warning("Languages fetch forbidden for %s/%s", owner, repo) return {} resp.raise_for_status() return resp.json() @@ -183,6 +201,10 @@ async def fetch_repo_tree( 加え、tree 取得自体が失敗した場合(非200 / 不正レスポンス / ``httpx.HTTPError``)も 「依存ゼロ」と「走査不能」を区別するため ``True`` を返す(D9(d))。不正 owner/repo は 実在リポではなく走査対象ですらないため ``([], False)`` とする。 + + ただしレート制限(403 + 残量 0 / 429)は 1 リポの部分走査ではなくトークン単位の + グローバルなスロットリングのため、partial として握り込まず ``RetryableError`` を raise し、 + リセット窓を待って連携タスク全体を再試行させる(#485。``fetch_repos_raw`` と同方針)。 """ if not _is_valid_owner_repo(owner, repo): return [], False @@ -192,7 +214,16 @@ async def fetch_repo_tree( f"/repos/{owner}/{repo}/git/trees/{branch}", params={"recursive": "1"}, ) + # レート制限は「1 リポの部分走査」ではなくトークン単位のスロットリングのため、 + # partial として握り込まず RetryableError で連携全体をリトライさせる(#485)。 + _raise_if_rate_limited(resp) if resp.status_code != 200: + logger.warning( + "Git tree fetch returned %s for %s/%s (partial)", + resp.status_code, + owner, + repo, + ) return [], True data = resp.json() if not isinstance(data, dict): @@ -228,6 +259,8 @@ async def fetch_repo_file( f"/repos/{owner}/{repo}/contents/{path}", headers={"Accept": "application/vnd.github.raw+json"}, ) + # レート制限は握り込まず RetryableError で連携全体をリトライさせる(#485) + _raise_if_rate_limited(resp) if resp.status_code != 200: return None return resp.text diff --git a/backend/tests/test_github_api_client.py b/backend/tests/test_github_api_client.py index 9161bbd4..610cdd1b 100644 --- a/backend/tests/test_github_api_client.py +++ b/backend/tests/test_github_api_client.py @@ -9,7 +9,13 @@ from unittest.mock import AsyncMock, MagicMock import httpx -from app.services.intelligence.github.api_client import fetch_repo_tree +import pytest +from app.services.intelligence.github.api_client import ( + fetch_languages, + fetch_repo_file, + fetch_repo_tree, +) +from app.services.tasks.exceptions import RetryableError def _run(coro): @@ -23,12 +29,24 @@ def _run(coro): def _client_with_tree(tree, truncated=False, status_code=200): """Trees API レスポンスを返す AsyncClient モックを生成する。""" resp = MagicMock(status_code=status_code) + resp.headers = {} resp.json = MagicMock(return_value={"tree": tree, "truncated": truncated}) client = MagicMock() client.get = AsyncMock(return_value=resp) return client +def _client_with_status(status_code, headers=None): + """任意ステータス・ヘッダのレスポンスを返す AsyncClient モックを生成する。""" + resp = MagicMock(status_code=status_code) + resp.headers = headers or {} + resp.json = MagicMock(return_value={}) + resp.text = "" + client = MagicMock() + client.get = AsyncMock(return_value=resp) + return client + + def test_returns_all_blob_paths(): """blob の相対パスをすべて返し、ディレクトリ(tree)は除外すること。""" tree = [ @@ -74,3 +92,69 @@ def test_invalid_owner_repo_returns_empty(): result = _run(fetch_repo_tree(client, "../evil", "repo", "main")) assert result == ([], False) client.get.assert_not_called() + + +def test_rate_limited_403_raises_retryable(): + """レート制限の 403(X-RateLimit-Remaining:0)は partial 握り込みではなく + RetryableError を raise し、連携全体をリトライ経路へ乗せること(#485)。""" + client = _client_with_status( + 403, headers={"x-ratelimit-remaining": "0", "retry-after": "42"} + ) + with pytest.raises(RetryableError) as exc: + _run(fetch_repo_tree(client, "u", "repo", "main")) + assert exc.value.retry_after == 42 + + +def test_429_raises_retryable(): + """429 Too Many Requests も RetryableError(retry_after 付き)で raise すること(#485)。""" + client = _client_with_status(429, headers={"retry-after": "30"}) + with pytest.raises(RetryableError) as exc: + _run(fetch_repo_tree(client, "u", "repo", "main")) + assert exc.value.retry_after == 30 + + +def test_genuine_403_returns_partial(): + """レート制限でない 403(残量あり = 権限エラー等)はリトライさせず + 従来どおり partial 扱い([], True)で返すこと(#485)。""" + client = _client_with_status(403, headers={"x-ratelimit-remaining": "57"}) + assert _run(fetch_repo_tree(client, "u", "repo", "main")) == ([], True) + + +def test_languages_rate_limited_raises_retryable(): + """fetch_languages もレート制限 403 を {} で握り込まず RetryableError を raise すること + (同一ホットパスの兄弟 fetch も #485 で統一)。""" + client = _client_with_status(403, headers={"x-ratelimit-remaining": "0"}) + with pytest.raises(RetryableError): + _run(fetch_languages(client, "u", "repo")) + + +def test_languages_genuine_403_returns_empty(): + """レート制限でない 403 は言語情報を欠いたまま {} で best-effort 継続すること。""" + client = _client_with_status(403, headers={"x-ratelimit-remaining": "57"}) + assert _run(fetch_languages(client, "u", "repo")) == {} + + +def test_repo_file_rate_limited_403_raises_retryable(): + """fetch_repo_file もレート制限 403 を None で握り込まず RetryableError を raise すること + (同一ホットパスの兄弟 fetch も #485 で統一)。""" + client = _client_with_status( + 403, headers={"x-ratelimit-remaining": "0", "retry-after": "42"} + ) + with pytest.raises(RetryableError) as exc: + _run(fetch_repo_file(client, "u", "repo", "requirements.txt")) + assert exc.value.retry_after == 42 + + +def test_repo_file_429_raises_retryable(): + """fetch_repo_file の 429 も RetryableError(retry_after 付き)で raise すること(#485)。""" + client = _client_with_status(429, headers={"retry-after": "30"}) + with pytest.raises(RetryableError) as exc: + _run(fetch_repo_file(client, "u", "repo", "requirements.txt")) + assert exc.value.retry_after == 30 + + +def test_repo_file_genuine_403_returns_none(): + """レート制限でない 403(残量あり = 権限エラー等)はリトライさせず + 従来どおり None(当該 manifest をスキップ)で best-effort 継続すること(#485)。""" + client = _client_with_status(403, headers={"x-ratelimit-remaining": "57"}) + assert _run(fetch_repo_file(client, "u", "repo", "requirements.txt")) is None