Skip to content

Remove Buffer Size in go-sh Shell Command #253

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
anisurrahman75 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Remove Buffer Size in go-sh Shell Command #253

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a71bdea
Remove Buffer Size
anisurrahman75 Feb 27, 2026
eab8d90
make gen
anisurrahman75 Feb 27, 2026
9819738
fix ci
anisurrahman75 Feb 27, 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
10 changes: 4 additions & 6 deletions pkg/restic/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package restic

import (
"bytes"
"encoding/json"
"errors"
"fmt"
Expand All @@ -30,7 +31,6 @@ import (

"stash.appscode.dev/apimachinery/apis/stash/v1alpha1"

"github.com/armon/circbuf"
"k8s.io/klog/v2"
storage "kmodules.xyz/objectstore-api/api/v1"
)
Expand Down Expand Up @@ -464,11 +464,9 @@ func (w *ResticWrapper) appendCaCertFlag(args []any) []any {

func (w *ResticWrapper) run(commands ...Command) ([]byte, error) {
// write std errors into os.Stderr and buffer
errBuff, err := circbuf.NewBuffer(256)
if err != nil {
return nil, err
}
w.sh.Stderr = io.MultiWriter(os.Stderr, errBuff)
var err error
var errBuff bytes.Buffer
w.sh.Stderr = io.MultiWriter(os.Stderr, &errBuff)
Comment thread
anisurrahman75 marked this conversation as resolved.

for _, cmd := range commands {
if cmd.Name == ResticCMD {
Expand Down
16 changes: 7 additions & 9 deletions pkg/restic/retry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ func NewRetryConfig() *RetryConfig {
return false
}
combined := strings.ToLower(err.Error() + " " + output)
klog.Infoln("Combined output: " + combined)
for _, pattern := range retryablePatterns {
if strings.Contains(combined, strings.ToLower(pattern)) {
return true
Expand All @@ -64,32 +63,31 @@ func NewRetryConfig() *RetryConfig {
func (rc *RetryConfig) RunWithRetry(ctx context.Context, execFunc func() ([]byte, error)) ([]byte, error) {
var output []byte
var lastErr error
attempts := 0
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
Comment on lines +78 to 83

Copilot AI Feb 27, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
klog.Infoln("Retrying command after error",
"attempt", attempts,
"maxRetries", rc.MaxRetries,
"error", fmt.Sprintf("%s %s", lastErr, string(output)))
attempts++
return false, nil
Comment on lines +66 to 86

Copilot AI Feb 27, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
},
)
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)

Copilot AI Feb 27, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
}

return output, lastErr
}