Conversation
mgreau
left a comment
There was a problem hiding this comment.
The retry logic pipeline feature is well-implemented overall with good documentation and test coverage. However, there are a few issues that should be addressed: a potential integer overflow in the exponential backoff calculation for high attempt numbers, a missing context parameter in a logging call, and a minor inconsistency in validation behavior.
mgreau
left a comment
There was a problem hiding this comment.
The PR adds a well-documented retry logic feature for pipelines with configurable backoff strategies. However, there are a few issues that should be addressed:
-
Potential integer overflow in exponential backoff calculation - The bit shift operation can overflow with high attempt numbers.
-
Interactive debug mode triggers on every retry attempt - The documentation states debug prompts should only appear on final failure, but the current implementation will trigger debug on every failed attempt since
maybeDebugis called insiderunPipelinebefore retry logic can intercept it. -
Missing context in clog.Warnf call - The warning for high retry attempts uses
clog.Warnfwithout a context parameter.
The implementation includes good test coverage, comprehensive documentation, and proper validation of retry configuration.
| multiplier := 1 << attemptNum | ||
| delay = time.Duration(multiplier) * initialDelay | ||
| default: | ||
| // Default to exponential | ||
| multiplier := 1 << attemptNum | ||
| delay = time.Duration(multiplier) * initialDelay | ||
| } | ||
|
|
||
| return min(delay, maxDelay) | ||
| } |
There was a problem hiding this comment.
ERROR: Potential integer overflow in exponential backoff calculation. When attemptNum is >= 63 on 64-bit systems (or >= 31 on 32-bit), the bit shift 1 << attemptNum will overflow. While validation warns at 10 attempts, the function should protect against this edge case.
| multiplier := 1 << attemptNum | |
| delay = time.Duration(multiplier) * initialDelay | |
| default: | |
| // Default to exponential | |
| multiplier := 1 << attemptNum | |
| delay = time.Duration(multiplier) * initialDelay | |
| } | |
| return min(delay, maxDelay) | |
| } | |
| // calculateBackoff calculates the backoff delay for a given retry attempt. | |
| func calculateBackoff(strategy string, attemptNum int, initialDelay, maxDelay time.Duration) time.Duration { | |
| var delay time.Duration | |
| switch strategy { | |
| case "constant": | |
| delay = initialDelay | |
| case "linear": | |
| delay = time.Duration(attemptNum+1) * initialDelay | |
| case "exponential": | |
| // 2^attemptNum * initialDelay, with overflow protection | |
| if attemptNum >= 62 { | |
| delay = maxDelay | |
| } else { | |
| multiplier := 1 << attemptNum | |
| delay = time.Duration(multiplier) * initialDelay | |
| } | |
| default: | |
| // Default to exponential | |
| if attemptNum >= 62 { | |
| delay = maxDelay | |
| } else { | |
| multiplier := 1 << attemptNum | |
| delay = time.Duration(multiplier) * initialDelay | |
| } | |
| } | |
| return min(delay, maxDelay) | |
| } |
| clog.Warnf("retry attempts set to %d, which may cause long build times", retry.Attempts) | ||
| } | ||
|
|
||
| // Validate backoff strategy |
There was a problem hiding this comment.
ERROR: The clog.Warnf function is called without a context parameter. This may not work correctly depending on how clog is configured. The function should either accept a context parameter or use a context-aware logging approach.
| clog.Warnf("retry attempts set to %d, which may cause long build times", retry.Attempts) | |
| } | |
| // Validate backoff strategy | |
| if retry.Attempts > 10 { | |
| // This is just a warning - validation still passes | |
| // Note: Using fmt.Printf as we don't have context here; consider passing ctx to this function | |
| fmt.Fprintf(os.Stderr, "warning: retry attempts set to %d, which may cause long build times\n", retry.Attempts) | |
| } |
Melange Pull Request Template
This PR is a copy of chainguard-dev#2300 for testing the code-reviewer CLI.
Functional Changes
Notes:
SCA Changes
Notes:
Linter
Notes: