Skip to content

fix(stability): close mkstemp fd, add request timeouts, fix mutable default arg #2367

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

Open
jclee941 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(stability): close mkstemp fd, add request timeouts, fix mutable default arg #2367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38f43b2
fix(stability): close mkstemp fd, add request timeouts, use context m…
May 3, 2026
c913534
fix(stability): replace bare except, add timeout on rate-limit calls
May 3, 2026
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
4 changes: 3 additions & 1 deletion pr_agent/algo/token_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class TokenHandler:
CLAUDE_MODEL = "claude-3-7-sonnet-20250219"
CLAUDE_MAX_CONTENT_SIZE = 9_000_000 # Maximum allowed content size (9MB) for Claude API

def __init__(self, pr=None, vars: dict = {}, system="", user=""):
def __init__(self, pr=None, vars: dict = None, system="", user=""):
"""
Initializes the TokenHandler object.

Expand All @@ -66,6 +66,8 @@ def __init__(self, pr=None, vars: dict = {}, system="", user=""):
- system: The system string.
- user: The user string.
"""
if vars is None:
vars = {}
self.encoder = TokenEncoder.get_token_encoder()

if pr is not None:
Expand Down
11 changes: 6 additions & 5 deletions pr_agent/algo/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1204,15 +1204,16 @@ def get_rate_limit_status(github_token) -> dict:
"Authorization": f"token {github_token}"
}

response = requests.get(RATE_LIMIT_URL, headers=HEADERS)
response = requests.get(RATE_LIMIT_URL, headers=HEADERS, timeout=10)
try:
rate_limit_info = response.json()
if rate_limit_info.get('message') == 'Rate limiting is not enabled.': # for github enterprise
return {'resources': {}}
response.raise_for_status() # Check for HTTP errors
except: # retry
except Exception: # retry
get_logger().warning("Rate limit check failed, retrying once", exc_info=True)
time.sleep(0.1)
response = requests.get(RATE_LIMIT_URL, headers=HEADERS)
response = requests.get(RATE_LIMIT_URL, headers=HEADERS, timeout=10)
return response.json()
Comment on lines +1207 to 1217
Copy link
Copy Markdown
Contributor

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot May 3, 2026

Choose a reason for hiding this comment

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

Action required

1. Rate-limit retry not executed 🐞 Bug ☼ Reliability

In get_rate_limit_status(), the initial requests.get() happens before the try/except, so
timeouts/connection failures bypass the intended "retry once" logic and warning log. With the new
timeout=10, this becomes a likely runtime failure mode (and upstream callers either crash or skip
rate-limit validation depending on which wrapper calls it).
Agent Prompt 
### Issue description
`get_rate_limit_status()` intends to retry once and log on failure, but `requests.get()` is executed before the `try:` so request-layer exceptions (timeout/connection) are not caught and the retry/log never run.

### Issue Context
This regression becomes more visible after adding `timeout=10` because timeouts will now be raised rather than hanging.

### Fix Focus Areas
- pr_agent/algo/utils.py[1198-1218]

### Suggested change
- Move the initial `requests.get(...)` into the `try:` block, or add an outer `try/except requests.RequestException` around both GET calls.
- On request exceptions, log once (with `exc_info=True`), sleep, retry once, and if the retry fails either:
  - re-raise a clear exception, or
  - return a safe sentinel value and let callers handle it (but don’t silently treat it as “rate limit OK” unless that is explicitly desired).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

return rate_limit_info

Expand Down Expand Up @@ -1249,8 +1250,8 @@ def validate_and_await_rate_limit(github_token):
time.sleep(sleep_time_sec + 1)
rate_limit_status = get_rate_limit_status(github_token)
return rate_limit_status
except:
get_logger().error("Error in rate limit")
except Exception:
get_logger().error("Error in rate limit", exc_info=True)
return None


Expand Down
3 changes: 2 additions & 1 deletion pr_agent/git_providers/gerrit_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ def upload_patch(patch, path):
headers={
"Content-Type": "application/json",
"Authorization": f"Bearer {patch_server_token}",
}
},
timeout=30,
)
response.raise_for_status()
patch_server_endpoint = patch_server_endpoint.rstrip("/")
Expand Down
5 changes: 4 additions & 1 deletion pr_agent/git_providers/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ def apply_repo_settings(pr_url):
category = 'local'
try:
fd, repo_settings_file = tempfile.mkstemp(suffix='.toml')
os.write(fd, repo_settings)
try:
os.write(fd, repo_settings)
finally:
os.close(fd)

try:
dynconf_kwargs = {'core_loaders': [], # DISABLE default loaders, otherwise will load toml files more than once.
Expand Down
9 changes: 5 additions & 4 deletions pr_agent/servers/bitbucket_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,13 @@ async def get_bearer_token(shared_secret: str, client_key: str):
@router.get("/")
async def handle_manifest(request: Request, response: Response):
cur_dir = os.path.dirname(os.path.abspath(__file__))
manifest = open(os.path.join(cur_dir, "atlassian-connect.json"), "rt").read()
with open(os.path.join(cur_dir, "atlassian-connect.json"), "rt") as f:
manifest = f.read()
try:
manifest = manifest.replace("app_key", get_settings().bitbucket.app_key)
manifest = manifest.replace("base_url", get_settings().bitbucket.base_url)
except:
get_logger().error("Failed to replace api_key in Bitbucket manifest, trying to continue")
except Exception:
get_logger().error("Failed to replace api_key in Bitbucket manifest, trying to continue", exc_info=True)
manifest_obj = json.loads(manifest)
return JSONResponse(manifest_obj)

Expand Down Expand Up @@ -101,7 +102,7 @@ async def _validate_time_from_last_commit_to_pr_update(data: dict) -> bool:
'Authorization': f'Bearer {bearer_token}',
'Accept': 'application/json'
}
response = requests.get(commits_api, headers=headers)
response = requests.get(commits_api, headers=headers, timeout=30)
if response.status_code != 200:
get_logger().warning(f"Bitbucket commits API returned {response.status_code} for {commits_api}")
return False
Expand Down
2 changes: 1 addition & 1 deletion pr_agent/servers/github_polling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ async def is_valid_notification(notification, headers, handled_ids, session, use
else: # we could not find the user tag in the latest comment. Check previous comments
# get all comments in the PR
requests_url = f"{pr_url}/comments".replace("pulls", "issues")
comments_response = requests.get(requests_url, headers=headers)
comments_response = requests.get(requests_url, headers=headers, timeout=30)
comments = comments_response.json()[::-1]
max_comment_to_scan = 4
for comment in comments[:max_comment_to_scan]:
Expand Down
Loading