Correct logic error in waiting, and do not assume server time is in sync with local machine time

Author: holly-cummins

src/main/java/org/kohsuke/github/AbuseLimitHandler.java

@@ -5,6 +5,7 @@
 import java.io.IOException;
 import java.io.InterruptedIOException;
 import java.net.HttpURLConnection;
+import java.time.Duration;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
 import java.time.temporal.ChronoUnit;
@@ -25,6 +26,8 @@
 @Deprecated
 public abstract class AbuseLimitHandler extends GitHubAbuseLimitHandler {
 
+    private static final int MINIMUM_ABUSE_RETRY_MILLIS = 1000;
+
     /**
      * Called when the library encounters HTTP error indicating that the API abuse limit is reached.
      *
@@ -110,15 +113,26 @@ long parseWaitTime(HttpURLConnection uc) {
         String v = uc.getHeaderField("Retry-After");
         if (v == null) {
             // can't tell, wait for unambiguously over one minute per GitHub guidance
-            return 61 * 1000;
+            return Duration.ofSeconds(61).toMillis();
         }
 
         try {
-            return Math.max(1000, Long.parseLong(v) * 1000);
+            return Math.max(MINIMUM_ABUSE_RETRY_MILLIS, Duration.ofSeconds(Long.parseLong(v)).toMillis());
         } catch (NumberFormatException nfe) {
             // The retry-after header could be a number in seconds, or an http-date
+            // We know it was a date if we got a number format exception :)
+
+            // Don't use ZonedDateTime.now(), because the local and remote server times may not be in sync
+            // Instead, we can take advantage of the Date field in the response to see what time the remote server thinks it is
+            String dateField = uc.getHeaderField("Date");
+            ZonedDateTime now;
+            if (dateField != null) {
+                now = ZonedDateTime.parse(dateField, DateTimeFormatter.RFC_1123_DATE_TIME);
+            } else {
+                 now = ZonedDateTime.now();
+            }
             ZonedDateTime zdt = ZonedDateTime.parse(v, DateTimeFormatter.RFC_1123_DATE_TIME);
-            return ChronoUnit.MILLIS.between(ZonedDateTime.now(), zdt);
+            return Math.max(MINIMUM_ABUSE_RETRY_MILLIS, ChronoUnit.MILLIS.between(now, zdt));
         }
     }
 

src/main/java/org/kohsuke/github/GHRateLimit.java

@@ -527,7 +527,7 @@ && getRemaining() <= other.getRemaining())) {
                 return this;
             } else if (!(other instanceof UnknownLimitRecord)) {
                 // If the above is not the case that means other has a later reset
-                // or the same resent and fewer requests remaining.
+                // or the same reset and fewer requests remaining.
                 // If the other record is not an unknown record, the other is more recent
                 return other;
             } else if (this.isExpired() && !other.isExpired()) {

src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java

@@ -18,6 +18,7 @@
 import static org.hamcrest.CoreMatchers.notNullValue;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.core.IsInstanceOf.instanceOf;
+import static org.junit.Assert.assertEquals;
 
 // TODO: Auto-generated Javadoc
 /**
@@ -476,8 +477,7 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
                         // logic
                         // Manually invoke it to make sure it's what we intended
                         long waitTime = parseWaitTime(uc);
-                        // The exact value here will depend on when the test is run, but it should be positive, and huge
-                        assertThat(waitTime, greaterThan(1000 * 1000l));
+                        assertEquals(waitTime, 7*60*1000);
 
                         AbuseLimitHandler.FAIL.onError(e, uc);
                     }
@@ -537,7 +537,8 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
                         // logic
                         // Manually invoke it to make sure it's what we intended
                         long waitTime = parseWaitTime(uc);
-                        assertThat(waitTime, greaterThan(60000l));
+                        // We expect to wait for 61s if there's no information from the server
+                        assertEquals(waitTime, 61000l);
 
                         AbuseLimitHandler.FAIL.onError(e, uc);
                     }

src/test/resources/org/kohsuke/github/AbuseLimitHandlerTest/wiremock/testHandler_Wait_Secondary_Limits_Too_Many_Requests_Date_Retry_After/mappings/3-r_h_t_fail.json

@@ -14,11 +14,11 @@
     "status": 429,
     "body": "{\"message\":\"You have exceeded a secondary rate limit. Please wait a few minutes before you try again\"}",
     "headers": {
-      "Date": "{{now timezone='GMT' format='EEE, dd MMM yyyy HH:mm:ss z'}}",
+      "Date": "Mon, 21 Oct 2024 07:27:00 GMT",
       "Content-Type": "application/json; charset=utf-8",
       "Server": "GitHub.com",
       "Status": "429 Too Many Requests",
-      "Retry-After": "Mon, 21 Oct 2115 07:28:00 GMT",
+      "Retry-After": "Mon, 21 Oct 2024 07:34:00 GMT",
       "Cache-Control": "private, max-age=60, s-maxage=60",
       "Vary": [
         "Accept, Authorization, Cookie, X-GitHub-OTP",