fix(retry): address bala review on PR #1701 review 4248622939

Five concessions from bala's review batch — each verified against
minio-go's contract or the file's own pre-PR conventions:

BaseS3Client.java
- Restore the `protected static final Random RANDOM` field (with its
  java.security.SecureRandom + java.util.Random imports) that
  60aeaed dropped when switching backoff jitter to
  ThreadLocalRandom. Per bala's r3205617158: do not remove
  protected/public API surface. The field is part of the public
  subclassing contract on BaseS3Client and is the Java analogue of
  minio-go's per-client `c.random` source (api.go:307); SDK-internal
  code paths still use ThreadLocalRandom.current() for the retry
  backoff hot path so jitter is unchanged.
- Drop the null-guard around `regionCache.remove(s3request.bucket())`
  introduced in beead6e. The guard was only load-bearing for the
  contrived testNoRetryOn404 (listBuckets + 404 NoSuchBucket — a wire
  shape no spec-compliant S3 server emits since NoSuchBucket only
  applies to bucket-scoped ops); the test is rewritten in this commit
  to a realistic scenario, removing the dependency on the guard.

Http.java
- Drop the two-line `// Our RetryInterceptor handles transient
  failures with full-jitter backoff; defer entirely to it instead of
  layering OkHttp's connection-level retry on top.` comment from
  newDefaultClient() per bala's r3205589355: the
  retryOnConnectionFailure(false) call paired with addInterceptor(new
  RetryInterceptor()) is self-explanatory and the prose duplicated the
  RetryInterceptor Javadoc.
- Drop the `Utils.validateNotNull(requestBody, "request body")` guard
  from the Body(okhttp3.RequestBody) constructor per bala's
  r3205613399; the three other Body constructors do not validate
  non-null and let the next-line dereference fail with the JVM's
  helpful-NPE message — restoring that local convention.
- Drop the `private long fileOffset` field, its capture in the
  Body(RandomAccessFile,...) constructor, and the seek in
  toRequestBody() per bala's r3205614300. Empirical truth-table probe
  (4 trials toggling F = Body.fileOffset against C+R = createBody
  save/restore + RequestBody.position+seek) confirmed F is redundant
  when C+R are present: removing F alone keeps both first-attempt and
  retry-attempt body content correct (Trial 3), since the
  pre-Body-construction seek in createBody already places the file
  pointer where RequestBody.position will capture it on construction
  and writeTo() will seek back to on each network attempt.

RetryTest.java
- Rewrite testNoRetryOn404 to call removeBucket on a non-existent
  bucket. NoSuchBucket only applies to bucket-scoped operations on
  the S3 wire; pairing it with the bucket-less listBuckets() is a
  contrived scenario AWS S3 / MinIO never produce. The rewrite
  preserves coverage (404 does not retry, ErrorResponseException
  carries code "NoSuchBucket" and status 404, request count == 1)
  while exercising a wire shape the SDK actually has to handle, and
  uses a non-null bucket so the regionCache invalidation path runs
  with a real ConcurrentHashMap.remove(String) call.

Validated locally on Java 17:
- :api:check (compile + spotless + spotbugs + tests) PASS
- :api:javadoc PASS

Author: allanrogerr

api/src/main/java/io/minio/BaseS3Client.java

@@ -51,11 +51,13 @@
 import java.io.OutputStreamWriter;
 import java.io.PrintWriter;
 import java.nio.charset.StandardCharsets;
+import java.security.SecureRandom;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Random;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
@@ -90,6 +92,7 @@ public abstract class BaseS3Client implements AutoCloseable {
       "ServerSideEncryptionConfigurationNotFoundError";
   // maximum allowed bucket policy size is 20KiB
   protected static final int MAX_BUCKET_POLICY_SIZE = 20 * 1024;
+  protected static final Random RANDOM = new Random(new SecureRandom().nextLong());
   protected static final ObjectMapper OBJECT_MAPPER =
       JsonMapper.builder()
           .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
@@ -512,10 +515,9 @@ private void onResponse(final Response response) throws IOException {
                           response.header("x-amz-id-2"));
                 }
 
-                // invalidate region cache if needed (bucket may be null for e.g. listBuckets)
-                if (s3request.bucket() != null
-                    && (errorResponse.code().equals(NO_SUCH_BUCKET)
-                        || errorResponse.code().equals(RETRY_HEAD))) {
+                // invalidate region cache if needed
+                if (errorResponse.code().equals(NO_SUCH_BUCKET)
+                    || errorResponse.code().equals(RETRY_HEAD)) {
                   regionCache.remove(s3request.bucket());
                 }
 

api/src/main/java/io/minio/Http.java

@@ -623,8 +623,6 @@ public static OkHttpClient newDefaultClient() {
             .writeTimeout(DEFAULT_TIMEOUT, TimeUnit.MILLISECONDS)
             .readTimeout(DEFAULT_TIMEOUT, TimeUnit.MILLISECONDS)
             .protocols(Arrays.asList(Protocol.HTTP_1_1))
-            // Our RetryInterceptor handles transient failures with full-jitter backoff; defer
-            // entirely to it instead of layering OkHttp's connection-level retry on top.
             .retryOnConnectionFailure(false)
             .addInterceptor(new RetryInterceptor())
             .build();
@@ -846,7 +844,6 @@ private static String peekS3ErrorCode(okhttp3.Response response) {
   public static class Body {
     private okhttp3.RequestBody requestBody;
     private RandomAccessFile file;
-    private long fileOffset;
     private ByteBuffer buffer;
     private byte[] data;
     private Long length;
@@ -864,7 +861,7 @@ public static class Body {
      * disabled for that call.
      */
     public Body(okhttp3.RequestBody requestBody) {
-      this.requestBody = Utils.validateNotNull(requestBody, "request body");
+      this.requestBody = requestBody;
       this.contentType = requestBody.contentType();
     }
 
@@ -877,11 +874,6 @@ public Body(
         String md5Hash) {
       if (length < 0) throw new IllegalArgumentException("valid length must be provided");
       this.file = file;
-      try {
-        this.fileOffset = file.getFilePointer();
-      } catch (IOException e) {
-        throw new IllegalStateException("failed to read file position", e);
-      }
       set(length, contentType, sha256Hash, md5Hash);
     }
 
@@ -955,14 +947,7 @@ public Headers headers() {
     /** Creates HTTP RequestBody for this body. */
     public RequestBody toRequestBody() throws MinioException {
       if (requestBody != null) return new RequestBody(requestBody);
-      if (file != null) {
-        try {
-          file.seek(fileOffset);
-        } catch (IOException e) {
-          throw new MinioException(e);
-        }
-        return new RequestBody(file, length, contentType);
-      }
+      if (file != null) return new RequestBody(file, length, contentType);
       if (buffer != null) return new RequestBody(buffer, contentType);
       return new RequestBody(data, length.intValue(), contentType);
     }

api/src/test/java/io/minio/RetryTest.java

@@ -282,6 +282,9 @@ public void testRetryOnRetryableS3CodeIn400Body() throws IOException, MinioExcep
 
   @Test
   public void testNoRetryOn404() throws IOException, MinioException {
+    // Bucket-scoped operation returning 404 NoSuchBucket — a wire-realistic shape
+    // (NoSuchBucket only ever applies to bucket-scoped calls). Verifies that 404
+    // does not trigger retry and that the parsed error surfaces both code and status.
     try (MockWebServer server = new MockWebServer()) {
       server.enqueue(xmlError(404, "NoSuchBucket"));
       server.start();
@@ -290,7 +293,8 @@ public void testNoRetryOn404() throws IOException, MinioException {
           MinioClient.builder().endpoint(server.url("").toString()).maxRetries(3).build();
       try {
         try {
-          client.listBuckets();
+          client.removeBucket(
+              RemoveBucketArgs.builder().bucket("missing-bucket-for-no-retry").build());
           Assert.fail("expected ErrorResponseException");
         } catch (ErrorResponseException e) {
           Assert.assertEquals("NoSuchBucket", e.errorResponse().code());