Skip to content

fix: adding rate limit warning at verify token permission stage #14756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kangyan-Zhou merged 1 commit into from
Dec 10, 2025
Merged

fix: adding rate limit warning at verify token permission stage #14756

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions scripts/ci/publish_traces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@
from urllib.request import Request, urlopen


def is_rate_limit_error(e):
"""Check if an exception is a rate limit error"""
return (
isinstance(e, HTTPError)
and e.code in [403, 429]
and "rate limit exceeded" in getattr(e, "error_body", "").lower()
)


def make_github_request(url, token, method="GET", data=None):
"""Make authenticated request to GitHub API"""
headers = {
Expand Down Expand Up @@ -56,6 +65,9 @@ def verify_token_permissions(repo_owner, repo_name, token):
repo_data = json.loads(response)
print(f"Repository access verified: {repo_data['full_name']}")
except Exception as e:
if is_rate_limit_error(e):
warnings.warn("GitHub API rate limit exceeded during token verification.")
return "rate_limited"
print(f"Failed to access repository: {e}")
return False

Expand All @@ -65,6 +77,9 @@ def verify_token_permissions(repo_owner, repo_name, token):
response = make_github_request(url, token)
print("Repository contents access verified")
except Exception as e:
if is_rate_limit_error(e):
warnings.warn("GitHub API rate limit exceeded during token verification.")
return "rate_limited"
print(f"Failed to access repository contents: {e}")
return False
Comment on lines 79 to 84
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This exception handling block is largely a duplicate of the one at lines 67-72. To adhere to the Don't Repeat Yourself (DRY) principle, you could extract this logic into a helper function. This would make the code cleaner and easier to maintain.

For example:

def _handle_verification_error(e, failure_message):
    if is_rate_limit_error(e):
        warnings.warn("GitHub API rate limit exceeded during token verification.")
        return "rate_limited"
    print(f"{failure_message}: {e}")
    return False

You could then call this helper from both except blocks in verify_token_permissions:

# ...
except Exception as e:
    return _handle_verification_error(e, "Failed to access repository")
# ...
except Exception as e:
    return _handle_verification_error(e, "Failed to access repository contents")


Expand Down Expand Up @@ -277,7 +292,14 @@ def publish_traces(traces_dir, run_id, run_number):
print(f"Found {len(files_to_upload)} files to upload")

# Verify token permissions before proceeding
if not verify_token_permissions(repo_owner, repo_name, token):
permission_check = verify_token_permissions(repo_owner, repo_name, token)
if permission_check == "rate_limited":
warnings.warn(
"Skipping trace upload due to GitHub API rate limit. "
"This is expected during high CI activity and does not indicate a test failure."
)
return
elif not permission_check:
Comment on lines +295 to +302
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The function verify_token_permissions now returns mixed types (boolean and string), which can make the code harder to reason about. The check elif not permission_check: correctly handles the False case, but it relies on the truthiness of different return types, which can be less intuitive to read than explicit comparisons.

To improve clarity and maintainability, I suggest using a consistent return type, for example by defining constants for the possible outcomes.

For example, you could define constants at the top of the file:

PERM_OK = "ok"
PERM_FAILED = "failed"
PERM_RATE_LIMITED = "rate_limited"

Then, verify_token_permissions would return these string values (e.g., PERM_OK instead of True, PERM_FAILED instead of False). The check here would become a more explicit and readable series of comparisons:

permission_check = verify_token_permissions(repo_owner, repo_name, token)
if permission_check == PERM_RATE_LIMITED:
    # ...
elif permission_check == PERM_FAILED:
    # ...

print(
"Token permission verification failed. Please check the token permissions."
)
Expand Down Expand Up @@ -340,11 +362,7 @@ def publish_traces(traces_dir, run_id, run_number):
error_type = f"HTTP {e.code}"

# Check for rate limit errors (non-fatal - just warn and skip)
if (
isinstance(e, HTTPError)
and e.code in [403, 429]
and "rate limit exceeded" in getattr(e, "error_body", "").lower()
):
if is_rate_limit_error(e):
warnings.warn("GitHub API rate limit exceeded. Skipping trace upload.")
return

Expand Down
Loading