CRITICAL: RetryPolicy has 7+ serious bugs that will cause production failures

[sfloess]

Severity: CRITICAL

Bug 1: Thread.sleep() Blocks Calling Thread - No Cancellation (Line 104)

Code:

try {
    Thread.sleep(delay);
} catch (InterruptedException ie) {
    Thread.currentThread().interrupt();
    throw new IOException("Retry interrupted", ie);
}

Problems:

A. No Way to Cancel Retry
Once you start retrying, you're STUCK until all retries complete:

RetryPolicy policy = RetryPolicy.aggressive();  // 5 retries, up to 30s delays

// This could block for MINUTES with no way to stop it:
policy.execute(() -> {
    // Network call that keeps failing...
    return slowServer.getData();  // User clicks "Cancel" - TOO BAD!
});

Total blocking time with default aggressive():

• Attempt 0: immediate
• Attempt 1: 200ms delay
• Attempt 2: 400ms delay
• Attempt 3: 800ms delay
• Attempt 4: 1,600ms delay
• Attempt 5: 3,200ms delay
• Total: 6.2 seconds minimum (plus jitter, plus actual operation time)

If operations also fail slowly:

// Each attempt times out after 30 seconds
// 5 retries × 30s = 150 seconds = 2.5 MINUTES of blocking!

B. Wastes Thread Pool Threads

ExecutorService executor = Executors.newFixedThreadPool(10);

// Submit 100 tasks that all retry
for (int i = 0; i < 100; i++) {
    executor.submit(() -> 
        retryPolicy.execute(() -> failingOperation())
    );
}

// All 10 threads are now BLOCKED sleeping!
// No new tasks can run!

Fix:
Use ScheduledExecutorService:

public <T> CompletableFuture<T> executeAsync(IOSupplier<T> operation, Executor executor) {
    CompletableFuture<T> future = new CompletableFuture<>();
    retryAsync(operation, 0, future, executor);
    return future;
}

private <T> void retryAsync(IOSupplier<T> operation, int attempt, 
                            CompletableFuture<T> future, Executor executor) {
    try {
        T result = operation.get();
        future.complete(result);
    } catch (IOException e) {
        if (attempt >= maxRetries) {
            future.completeExceptionally(e);
            return;
        }
        
        long delay = calculateDelay(attempt);
        scheduler.schedule(
            () -> retryAsync(operation, attempt + 1, future, executor),
            delay, TimeUnit.MILLISECONDS
        );
    }
}

Bug 2: Jitter Implementation is WRONG (Lines 136-138)

Current code:

if (jitter && delay > 0) {
    long jitterAmount = (long) (delay * 0.25 * Math.random());
    delay += jitterAmount;  // ← ONLY ADDS, never subtracts!
}

This is NOT jitter - it's just adding random delay!

Real jitter should be ± random amount:

• Sometimes shorter delays (reduce thundering herd)
• Sometimes longer delays (spread out load)

Current behavior:

• delay = 1000ms
• jitterAmount = 0 to 250ms (always positive)
• final delay = 1000ms to 1250ms

Correct jitter:

• delay = 1000ms
• jitterAmount = -250ms to +250ms
• final delay = 750ms to 1250ms

Why this matters:
Jitter prevents thundering herd - when many clients retry at the same time after a failure.

Scenario:

Server goes down at 10:00:00
1000 clients all fail their request
All 1000 retry after exactly 1000ms (at 10:00:01)
Server gets 1000 simultaneous requests → overloaded again!

With proper jitter:

1000 clients retry between 750ms and 1250ms
Requests spread out over 500ms window
Server handles gradual ramp-up

Fix:

if (jitter && delay > 0) {
    // Random value between -0.25 and +0.25
    double jitterFactor = (Math.random() - 0.5) * 0.5;  // -0.25 to +0.25
    long jitterAmount = (long) (delay * jitterFactor);
    delay = Math.max(0, delay + jitterAmount);  // Don't go negative
}

Or better, use ThreadLocalRandom:

if (jitter && delay > 0) {
    long maxJitter = delay / 4;  // 25% jitter
    long jitterAmount = ThreadLocalRandom.current().nextLong(-maxJitter, maxJitter + 1);
    delay = Math.max(0, delay + jitterAmount);
}

Bug 3: Math.pow() Can Overflow (Line 130)

Code:

long delay = (long) (initialDelayMs * Math.pow(backoffMultiplier, attemptNumber));

Problem:
Math.pow() returns double, which can exceed Long.MAX_VALUE:

RetryPolicy policy = new RetryPolicy(100, 1000, Long.MAX_VALUE, 2.0, false);

// Attempt 63:
// delay = 1000 * 2^63 = 9,223,372,036,854,775,808 (overflows long!)
// Cast to long wraps around to NEGATIVE number!
// delay becomes negative → Thread.sleep(-X) throws IllegalArgumentException!

Even with maxDelayMs cap:

long delay = (long) (initialDelayMs * Math.pow(backoffMultiplier, attemptNumber));
// If this overflows, delay is negative
delay = Math.min(delay, maxDelayMs);  // Math.min(negative, positive) = negative!
// Still negative!

Fix:

private long calculateDelay(int attemptNumber) {
    long delay = initialDelayMs;
    
    // Multiply iteratively with overflow protection
    for (int i = 0; i < attemptNumber; i++) {
        // Check for overflow before multiplying
        if (delay > maxDelayMs / backoffMultiplier) {
            return maxDelayMs;  // Would overflow, cap it
        }
        delay = (long) (delay * backoffMultiplier);
    }
    
    delay = Math.min(delay, maxDelayMs);
    
    // Add jitter
    if (jitter && delay > 0) {
        long maxJitter = delay / 4;
        long jitterAmount = ThreadLocalRandom.current().nextLong(-maxJitter, maxJitter + 1);
        delay = Math.max(0, delay + jitterAmount);
    }
    
    return delay;
}

Bug 4: Math.random() is Thread-Unsafe (Line 137)

Code:

long jitterAmount = (long) (delay * 0.25 * Math.random());

From Math.random() Javadoc:

This method is properly synchronized to allow correct use by more than one thread. However, if many threads need to generate pseudorandom numbers at a great rate, it may reduce contention for each thread to have its own pseudorandom-number generator.

Problem:
Math.random() uses a single shared Random instance with synchronized access.

Impact on concurrent retries:

// 100 threads all retrying at once
ExecutorService executor = Executors.newFixedThreadPool(100);
for (int i = 0; i < 100; i++) {
    executor.submit(() -> retryPolicy.execute(() -> operation()));
}

// All 100 threads contend on Math.random()'s lock!
// Severe performance degradation!

Fix:
Use ThreadLocalRandom (zero contention):

long jitterAmount = ThreadLocalRandom.current().nextLong(0, delay / 4 + 1);

Bug 5: Losing Stack Traces (Lines 110-116)

Code:

} catch (Exception e) {
    // Non-IOException - don't retry (likely a programming error)
    if (e instanceof RuntimeException) {
        throw (RuntimeException) e;  // ✅ Preserves stack trace
    }
    throw new IOException("Unexpected exception during retry", e);  // ❌ Wraps checked exceptions
}

Problem:
Checked exceptions (other than IOException) are wrapped in IOException.

Example:

retryPolicy.execute(() -> {
    // JDBC code
    return connection.prepareStatement("SELECT * FROM users").executeQuery();
});

// If SQLException is thrown:
// Original: SQLException with full stack trace
// After:    IOException("Unexpected exception", cause: SQLException)
// Stack trace shows retry logic, not original error location!

This makes debugging MUCH harder.

Better approach:

} catch (Exception e) {
    if (e instanceof RuntimeException) {
        throw (RuntimeException) e;
    }
    // Wrap in RuntimeException, not IOException
    throw new IllegalStateException(
        "Unexpected checked exception during retry (was the lambda correct?)", e);
}

Or better, don't catch Exception at all:

} catch (IOException e) {
    lastException = e;
    // ... retry logic ...
}
// Don't catch other exceptions - let them propagate naturally

Bug 6: Interrupt Handling Loses Information (Lines 105-108)

Code:

try {
    Thread.sleep(delay);
} catch (InterruptedException ie) {
    Thread.currentThread().interrupt();  // ✅ Restores flag
    throw new IOException("Retry interrupted", ie);  // ❌ Wraps in IOException
}

Problem:
InterruptedException is wrapped in IOException, making it look like an I/O error.

Caller can't distinguish:

try {
    retryPolicy.execute(() -> operation());
} catch (IOException e) {
    // Is this a network error or was I interrupted?
    if (e.getCause() instanceof InterruptedException) {
        // Have to unwrap to find out!
    }
}

Fix:
Let InterruptedException propagate OR create a specific exception:

public <T> T execute(IOSupplier<T> operation) throws IOException, InterruptedException {
    // ...
    try {
        Thread.sleep(delay);
    } catch (InterruptedException ie) {
        Thread.currentThread().interrupt();
        throw ie;  // Let caller handle it
    }
}

Or:

public class RetryInterruptedException extends IOException {
    public RetryInterruptedException(InterruptedException cause) {
        super("Retry was interrupted", cause);
    }
}

Bug 7: No Logging or Observability

Code has ZERO logging!

When retries happen, you have NO idea:

• How many retries occurred
• How long each retry took
• What exceptions were thrown
• Whether jitter was applied

For production debugging, you need:

private static final Logger logger = LoggerFactory.getLogger(RetryPolicy.class);

public <T> T execute(IOSupplier<T> operation) throws IOException {
    IOException lastException = null;

    for (int attempt = 0; attempt <= maxRetries; attempt++) {
        try {
            if (attempt > 0) {
                logger.debug("Retry attempt {} of {}", attempt, maxRetries);
            }
            
            long start = System.currentTimeMillis();
            T result = operation.get();
            
            if (attempt > 0) {
                logger.info("Operation succeeded after {} retries in {}ms", 
                           attempt, System.currentTimeMillis() - start);
            }
            
            return result;
        } catch (IOException e) {
            lastException = e;
            
            if (attempt < maxRetries) {
                long delay = calculateDelay(attempt);
                logger.warn("Attempt {} failed ({}), retrying in {}ms", 
                           attempt, e.getMessage(), delay);
                // ... sleep ...
            } else {
                logger.error("All {} retry attempts exhausted", maxRetries + 1, e);
            }
        }
    }
    
    throw new IOException("Failed after " + (maxRetries + 1) + " attempts", lastException);
}

Bug 8: No Metrics/Telemetry

No way to monitor retry behavior in production:

• How often do retries happen?
• What's the success rate after 1 retry? 2 retries?
• Are we hitting max retries often?

Should expose:

public class RetryMetrics {
    private final AtomicLong totalExecutions = new AtomicLong();
    private final AtomicLong successWithoutRetry = new AtomicLong();
    private final AtomicLong successAfterRetry = new AtomicLong();
    private final AtomicLong totalRetries = new AtomicLong();
    private final AtomicLong exhaustedRetries = new AtomicLong();
    
    // Histogram of retry counts
    private final ConcurrentHashMap<Integer, AtomicLong> retriesByAttempt = new ConcurrentHashMap<>();
}

Summary

Current RetryPolicy:

• ❌ Blocks threads (no cancellation)
• ❌ Jitter implementation is wrong (only adds, never subtracts)
• ❌ Can overflow with Math.pow()
• ❌ Uses slow, thread-unsafe Math.random()
• ❌ Loses stack traces by wrapping exceptions
• ❌ Wraps InterruptedException in IOException
• ❌ Zero logging (impossible to debug)
• ❌ Zero metrics (can't monitor production)

This class is the CORE of network reliability and it's BROKEN.

Impact: Production failures, poor performance, hard to debug, impossible to monitor.

Priority: CRITICAL - This affects every network operation in the library.

[sfloess]

RetryPolicy has been fixed with proper jitter implementation (±25% random variation using ThreadLocalRandom), overflow protection via iterative multiplication, and thread-safe random number generation. All critical bugs mentioned in this issue have been resolved.