From 8f7a652318bde90d24d3043cf1586e9a718f0f80 Mon Sep 17 00:00:00 2001
From: Paschalis Tsilias <tpaschalis@users.noreply.github.com>
Date: Thu, 20 Jun 2024 19:35:58 +0300
Subject: [PATCH] Amend Retry-After parsing logic for negative durations (#283)

It was wrong for parseHTTPDate to fail if response headers pointed to a time in the past. The correct action is to return 0 duration if the calculated duration is <=0, which is what this commit does.
---
 internal/retryafter.go      | 24 +++++++++++++++---------
 internal/retryafter_test.go | 13 +++++++++----
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/internal/retryafter.go b/internal/retryafter.go
index 784473bd..593cd2c9 100644
--- a/internal/retryafter.go
+++ b/internal/retryafter.go
@@ -18,26 +18,32 @@ type OptionalDuration struct {
 }
 
 func parseDelaySeconds(s string) (time.Duration, error) {
+	// Verify duration parsed properly
 	n, err := strconv.Atoi(s)
+	if err != nil {
+		return 0, errCouldNotParseRetryAfterHeader
+	}
 
-	// Verify duration parsed properly and bigger than 0
-	if err == nil && n > 0 {
+	// If n > 0 return n seconds, otherwise return 0
+	if n > 0 {
 		duration := time.Duration(n) * time.Second
 		return duration, nil
 	}
-	return 0, errCouldNotParseRetryAfterHeader
+	return 0, nil
 }
 
 func parseHTTPDate(s string) (time.Duration, error) {
+	// Verify duration parsed properly
 	t, err := http.ParseTime(s)
+	if err != nil {
+		return 0, errCouldNotParseRetryAfterHeader
+	}
 
-	// Verify duration parsed properly and bigger than 0
-	if err == nil {
-		if duration := time.Until(t); duration > 0 {
-			return duration, nil
-		}
+	// If the date is in the future return that duration, otherwise return 0
+	if duration := time.Until(t); duration > 0 {
+		return duration, nil
 	}
-	return 0, errCouldNotParseRetryAfterHeader
+	return 0, nil
 }
 
 // ExtractRetryAfterHeader extracts Retry-After response header if the status
diff --git a/internal/retryafter_test.go b/internal/retryafter_test.go
index f2e36e9f..123ada19 100644
--- a/internal/retryafter_test.go
+++ b/internal/retryafter_test.go
@@ -50,9 +50,10 @@ func TestExtractRetryAfterHeaderDelaySeconds(t *testing.T) {
 	resp.StatusCode = http.StatusBadGateway
 	assertUndefinedDuration(t, ExtractRetryAfterHeader(resp))
 
-	// Verify no duration is created for n < 0
+	// Verify a zero duration is created for n < 0
+	resp.StatusCode = http.StatusTooManyRequests
 	resp.Header.Set(retryAfterHTTPHeader, strconv.Itoa(-1))
-	assertUndefinedDuration(t, ExtractRetryAfterHeader(resp))
+	assertDuration(t, ExtractRetryAfterHeader(resp), 0)
 }
 
 func TestExtractRetryAfterHeaderHttpDate(t *testing.T) {
@@ -81,7 +82,11 @@ func TestExtractRetryAfterHeaderHttpDate(t *testing.T) {
 	resp.Header.Set(retryAfterHTTPHeader, retryAfter.Format(time.RFC1123))
 	assertUndefinedDuration(t, ExtractRetryAfterHeader(resp))
 
-	// Verify no duration is created for n < 0
+	// Verify a zero duration is created for n = 0
+	resp.Header.Set(retryAfterHTTPHeader, now.UTC().Format(http.TimeFormat))
+	assertDuration(t, ExtractRetryAfterHeader(resp), 0)
+
+	// Verify a zero duration is created for n < 0
 	resp.Header.Set(retryAfterHTTPHeader, now.Add(-1*time.Second).UTC().Format(http.TimeFormat))
-	assertUndefinedDuration(t, ExtractRetryAfterHeader(resp))
+	assertDuration(t, ExtractRetryAfterHeader(resp), 0)
 }