Skip to content

Conversation

@agrasth
Copy link
Contributor

@agrasth agrasth commented Oct 9, 2025

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • All static analysis checks passed.
  • This pull request is on the master branch.
  • I used gofmt for formatting the code before submitting the pull request.

1. Fixed Critical Retry Bug (jfrog-client-go/utils/io/content/contentreader.go)

Why: Enable actual retries by returning true for retryable Windows errors

Before:

return false, errorutils.CheckError(err)  // Never retries!

After:

if runtime.GOOS == "windows" {
    if pathErr, ok := err.(*os.PathError); ok {
        if errno, ok := pathErr.Err.(syscall.Errno); ok {
            // ERROR_SHARING_VIOLATION=32, ERROR_LOCK_VIOLATION=33, ERROR_ACCESS_DENIED=5
            if errno == 32 || errno == 33 || errno == 5 {
                return true, err  // ✅ Actually retry!
            }
        }
    }
}

Why check these error codes:

  • 32 (ERROR_SHARING_VIOLATION): File is open by another process
  • 33 (ERROR_LOCK_VIOLATION): File is locked
  • 5 (ERROR_ACCESS_DENIED): Can happen when antivirus is scanning

2. Added Exponential Backoff (jfrog-client-go/utils/retryexecutor.go)

Why: Customer has aggressive antivirus that may hold locks for longer periods

What:

  • Start: 100ms delay
  • Progression: 100ms → 200ms → 400ms → 800ms → 1s → 1s → 1s...
  • Max: 1000ms (as requested by customer requirements)
  • Total: Up to ~7.5 seconds across 10 retries

Code:

type RetryExecutor struct {
    MaxRetries               int
    RetriesIntervalMilliSecs int
    UseExponentialBackoff    bool  // NEW
    MaxBackoffMilliSecs      int   // NEW
    // ...
}

3. Increased Retry Count (both files)

Why: Handle aggressive antivirus that scans files longer

Change: 5 retries → 10 retries
Impact:

  • Before: ~1 second max total retry time (5 × 200ms)
  • After: ~7.5 seconds max total retry time (100+200+400+800+1000×6)

4. Applied Same Fix to RemoveTempDir (jfrog-client-go/utils/io/fileutils/temp.go)

Why: Same Windows file locking issue affects directory removal

Implementation: Created removeWithRetry() helper function with same logic

@agrasth agrasth requested a review from bhanurp October 9, 2025 07:58
@ehl-jf ehl-jf added the safe to test Approve running integration tests on a pull request label Oct 9, 2025
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 9, 2025
@ehl-jf ehl-jf added the safe to test Approve running integration tests on a pull request label Oct 13, 2025
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 13, 2025
@github-actions
Copy link
Contributor

👍 Frogbot scanned this pull request and did not find any new security issues.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants