- Notifications
You must be signed in to change notification settings - Fork 204
SG-41115 more changes#426
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.
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
base:ticket/SG-41115-minor-changes
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
e53655f85639c397910abad7821b7af09a03009c8fFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -495,8 +495,6 @@ class Shotgun(object): | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| _MULTIPART_UPLOAD_CHUNK_SIZE = 20000000 | ||||||||||||||||||||||||||||||||||||
| MAX_ATTEMPTS = 3 # Retries on failure | ||||||||||||||||||||||||||||||||||||
| BACKOFF = 0.75 # Seconds to wait before retry, times the attempt number | ||||||||||||||||||||||||||||||||||||
| def __init__( | ||||||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||||
| @@ -3757,8 +3755,16 @@ def _call_rpc( | ||||||||||||||||||||||||||||||||||||
| if self.config.localized is True: | ||||||||||||||||||||||||||||||||||||
| req_headers["locale"] = "auto" | ||||||||||||||||||||||||||||||||||||
| attempt = 1 | ||||||||||||||||||||||||||||||||||||
| while attempt <= self.MAX_ATTEMPTS: | ||||||||||||||||||||||||||||||||||||
| max_rpc_attempts = self.config.max_rpc_attempts | ||||||||||||||||||||||||||||||||||||
| rpc_attempt_interval = self.config.rpc_attempt_interval / 1000.0 | ||||||||||||||||||||||||||||||||||||
| attempt = 0 | ||||||||||||||||||||||||||||||||||||
| while attempt < max_rpc_attempts: | ||||||||||||||||||||||||||||||||||||
| if attempt: | ||||||||||||||||||||||||||||||||||||
| time.sleep(attempt * rpc_attempt_interval) | ||||||||||||||||||||||||||||||||||||
| attempt += 1 | ||||||||||||||||||||||||||||||||||||
| http_status, resp_headers, body = self._make_call( | ||||||||||||||||||||||||||||||||||||
| "POST", | ||||||||||||||||||||||||||||||||||||
| self.config.api_path, | ||||||||||||||||||||||||||||||||||||
| @@ -3776,10 +3782,8 @@ def _call_rpc( | ||||||||||||||||||||||||||||||||||||
| # We've seen some rare instances of PTR returning 502 for issues that | ||||||||||||||||||||||||||||||||||||
| # appear to be caused by something internal to PTR. We're going to | ||||||||||||||||||||||||||||||||||||
| # allow for limited retries for those specifically. | ||||||||||||||||||||||||||||||||||||
| if attempt != self.MAX_ATTEMPTS and e.errcode in [502, 504]: | ||||||||||||||||||||||||||||||||||||
| if attempt < max_rpc_attempts and e.errcode in [502, 504]: | ||||||||||||||||||||||||||||||||||||
| LOG.debug("Got a 502 or 504 response. Waiting and retrying...") | ||||||||||||||||||||||||||||||||||||
| time.sleep(float(attempt) * self.BACKOFF) | ||||||||||||||||||||||||||||||||||||
| attempt += 1 | ||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||
| elif e.errcode == 403: | ||||||||||||||||||||||||||||||||||||
| # 403 is returned with custom error page when api access is blocked | ||||||||||||||||||||||||||||||||||||
| @@ -3919,6 +3923,9 @@ def _make_call( | ||||||||||||||||||||||||||||||||||||
| rpc_attempt_interval = self.config.rpc_attempt_interval / 1000.0 | ||||||||||||||||||||||||||||||||||||
| while attempt < max_rpc_attempts: | ||||||||||||||||||||||||||||||||||||
| if attempt: | ||||||||||||||||||||||||||||||||||||
| time.sleep(attempt * rpc_attempt_interval) | ||||||||||||||||||||||||||||||||||||
| attempt += 1 | ||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||
| return self._http_request(verb, path, body, req_headers) | ||||||||||||||||||||||||||||||||||||
| @@ -3938,6 +3945,7 @@ def _make_call( | ||||||||||||||||||||||||||||||||||||
| if attempt == max_rpc_attempts: | ||||||||||||||||||||||||||||||||||||
| LOG.debug("Request failed. Giving up after %d attempts." % attempt) | ||||||||||||||||||||||||||||||||||||
| raise | ||||||||||||||||||||||||||||||||||||
| # TODO create only one attempt for SSL errors. | ||||||||||||||||||||||||||||||||||||
CopilotAI | ||||||||||||||||||||||||||||||||||||
| # TODO create only one attempt for SSL errors. | |
| # TODO: Fail immediately on SSL errors without retrying. |
CopilotAINov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'unitests' to 'unittests'.
| ## TODO - add unitests for those cases | |
| ## TODO - add unittests for those cases |
CopilotAINov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent loop condition: this uses attempt <= max_rpc_attempts while other retry loops in the same file use attempt < max_rpc_attempts (lines 3762, 3925, 4655). If max_rpc_attempts is 3, this loop will execute 4 times (attempt values 1, 2, 3, 4), while the others execute 3 times. This should be attempt < max_rpc_attempts for consistency.
| whileattempt<=max_rpc_attempts: | |
| whileattempt<max_rpc_attempts: |
CopilotAINov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The safe_short_url function lacks a docstring. Given its purpose of sanitizing URLs for logging (removing credentials and truncating paths), a docstring would improve maintainability by explaining the security and formatting rationale.
| defsafe_short_url(url: str, max_path_length: int=80) ->str: | |
| defsafe_short_url(url: str, max_path_length: int=80) ->str: | |
| """ | |
| SanitizeaURLforsafeloggingbyremovingsensitiveinformationandtruncatinglongpaths. | |
| Thisfunctionremovescredentials (suchasHTTPBasicAuthusernames/passwordsandAPIkeys) | |
| fromtheURL, stripsqueryparametersandfragments, andtruncatesthepathifitexceeds | |
| `max_path_length`byreplacingthemiddlepartwith'[...]'. Thishelpspreventaccidental | |
| exposureofsensitivedatainlogsorerrormessages. | |
| Args: | |
| url (str): TheURLtosanitize. | |
| max_path_length (int): Maximumallowedlengthforthepathcomponent. Defaultsto80. | |
| Returns: | |
| str: Thesanitized, shortenedURLsafeforlogging. | |
| """ |
Uh oh!
There was an error while loading. Please reload this page.
CopilotAINov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sleep duration calculation multiplies
attempt * rpc_attempt_interval, butattemptstarts at 0 and increments to 1 before the first iteration's logic. This means the first retry (after a failure) will sleep for1 * rpc_attempt_interval, the second retry for2 * rpc_attempt_interval, etc. However, in the old code, the sleep wasattempt * BACKOFFwhere attempt started at 1, giving the same progression. The issue is that whenattemptreachesmax_rpc_attempts, the loop exits, but the last attempt number logged will bemax_rpc_attempts. Ifmax_rpc_attemptsis 3, you'll see attempts 1, 2, 3, but only 3 actual iterations (0->1, 1->2, 2->3 before exit). This is correct behavior, but the new loop conditionattempt < max_rpc_attemptswithattemptstarting at 0 means ifmax_rpc_attemptsis 3, you get attempts numbered 1, 2, 3 (three attempts total), which matches the old behavior whereMAX_ATTEMPTS = 3gave attempts 1, 2, 3. No issue here on second thought, but worth verifying the intent is the same number of attempts.