Remove Buffer Size in go-sh Shell Command#253
Conversation
Signed-off-by: Anisur Rahman <anisur@appscode.com>
There was a problem hiding this comment.
Pull request overview
This PR adjusts how restic command stderr is buffered (removing the fixed-size circular buffer) and updates the retry loop behavior/logging used around command execution.
Changes:
- Replace fixed-size stderr circular buffer with an unbounded
bytes.Bufferin the restic command runner. - Modify retry attempt counting/termination logic and add per-attempt logging.
- Return captured output on retry failure instead of
nil.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pkg/restic/retry.go |
Changes retry attempt counting, logging, and error/output return behavior in RunWithRetry. |
pkg/restic/commands.go |
Replaces bounded stderr buffering (circbuf) with bytes.Buffer for shell command stderr capture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| attempts := 1 | ||
|
|
||
| err := wait.PollUntilContextCancel( | ||
| ctx, | ||
| rc.Delay, | ||
| true, // Run immediately on first call | ||
| func(ctx context.Context) (bool, error) { | ||
| // Stop if max retries reached | ||
| if attempts >= rc.MaxRetries { | ||
| if attempts > rc.MaxRetries { | ||
| return false, fmt.Errorf("max retries reached") | ||
| } | ||
| output, lastErr = execFunc() | ||
| klog.Infof("Attempt #%d: retrying in %v", attempts, rc.Delay) | ||
| klog.Infof("Attempt #%d error: %v", attempts, lastErr) | ||
| klog.Infof("Attempt #%d output: %s", attempts, string(output)) | ||
|
|
||
| if !rc.ShouldRetry(lastErr, string(output)) { | ||
| return true, nil | ||
| } | ||
| klog.Infoln("Retrying command after error", | ||
| "attempt", attempts, | ||
| "maxRetries", rc.MaxRetries, | ||
| "error", fmt.Sprintf("%s %s", lastErr, string(output))) | ||
| attempts++ | ||
| return false, nil |
There was a problem hiding this comment.
attempts accounting is off-by-one: starting at 1 and incrementing only when retrying means you can hit the attempts > rc.MaxRetries guard after incrementing, without actually executing that last "attempt". This can over-report the number of attempts in the final error message and effectively reduces/changes the intended semantics of MaxRetries.
Consider incrementing the counter only when execFunc() is called (or starting from 0 and using >=), and ensuring the reported attempt count matches the number of executions.
| klog.Infof("Attempt #%d: retrying in %v", attempts, rc.Delay) | ||
| klog.Infof("Attempt #%d error: %v", attempts, lastErr) | ||
| klog.Infof("Attempt #%d output: %s", attempts, string(output)) | ||
|
|
||
| if !rc.ShouldRetry(lastErr, string(output)) { | ||
| return true, nil |
There was a problem hiding this comment.
The new Info-level logs are misleading and potentially very noisy: "retrying in %v" is logged even when ShouldRetry returns false (i.e., when no retry will happen), and logging full output/error every attempt can leak sensitive data and produce very large logs.
Consider logging only inside the retry path, using a higher verbosity (e.g., klog.V(...)), and truncating/redacting command output before logging.
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed after %d attempts: %w", attempts, lastErr) | ||
| return output, fmt.Errorf("failed after %d attempts: %w", attempts, lastErr) |
There was a problem hiding this comment.
When polling ends with a non-nil err (context cancellation, poll internal error, or the "max retries reached" error), the returned error wraps lastErr instead of err. This can drop the real termination reason (e.g., context deadline exceeded) and can even wrap nil if no attempt was executed.
Consider returning/wrapping err (and optionally attaching lastErr as context) so callers can reliably detect cancellation vs command failure.
| return output, fmt.Errorf("failed after %d attempts: %w", attempts, lastErr) | |
| if lastErr != nil { | |
| return output, fmt.Errorf("failed after %d attempts: %w (last error: %v)", attempts, err, lastErr) | |
| } | |
| return output, fmt.Errorf("failed after %d attempts: %w", attempts, err) |
No description provided.