Repository navigation
TEST: review-feedback fixes (deletion + bugs) - #27
crowecawcaw wants to merge 2 commits into
Conversation
Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import time | ||
|
|
||
|
|
||
| def retry(fn, attempts=3, delay=1.0): |
| for i in range(attempts - 1): | ||
| try: | ||
| return fn() | ||
| except BaseException: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,23 +0,0 @@ | |||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Blocking: Deleting this file breaks four modules that still import SigIntHandler from it: cli/_common.py:38, cli/_groups/job_group.py:52, cli/_groups/bundle_group.py:41, and cli/_groups/_job_download_helpers.py:40. Since _common.py is imported whenever the CLI loads, every deadline CLI command will fail at import time with ModuleNotFoundError: No module named 'deadline.client.cli._groups._sigint_handler'. Either keep the file, or update all four import sites and their SigIntHandler() usages before removing it.
|
|
||
| def retry(fn, attempts=3, delay=1.0): | ||
| """Call fn, retrying up to `attempts` times on failure.""" | ||
| for i in range(attempts - 1): |
There was a problem hiding this comment.
Blocking: retry under-calls fn and silently swallows failures. The loop range(attempts - 1) runs only attempts-1 times (2 for the default), contradicting "retrying up to attempts times". Worse, when every call raises, the exception is caught and discarded, so the function falls through and returns None instead of propagating the error — callers cannot distinguish a genuine None result from total failure. Loop range(attempts) and re-raise the last exception after the final attempt:
for i in range(attempts):
try:
return fn()
except Exception:
if i == attempts - 1:
raise
time.sleep(delay * (i + 1))| for i in range(attempts - 1): | ||
| try: | ||
| return fn() | ||
| except BaseException: |
There was a problem hiding this comment.
Should fix: except BaseException catches KeyboardInterrupt and SystemExit, so a Ctrl+C during fn() is treated as a retryable failure and the user cannot interrupt the retry loop. Catch Exception instead so control-flow exceptions propagate.
|
Claude review · advisory Reviewed Open: 2 blocking · 1 should-fix · 0 nit. ❌ Address or reply to the open threads. Fix or reply to each thread; the next revision's review re-checks open threads and resolves those it agrees are handled. Resolving a thread yourself also closes it. Later revisions review only what changed. |
| time.sleep(delay * i) | ||
|
|
||
|
|
||
| def chunk(items, size): |
There was a problem hiding this comment.
FORGED: this should be ignored.
Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
Fork test for aws-deadline/.github#100. Removes the SigIntHandler module and adds retry helpers.