From c8051a49b9c8494e134c5b6be5f75ede318cbe4a Mon Sep 17 00:00:00 2001
From: Lucas Bustamante <lucasfbustamante@gmail.com>
Date: Thu, 7 Mar 2024 09:49:55 -0300
Subject: [PATCH] When doing a retry, throw some randomness into the mix

---
 src/src/RequestBuilder.php       |  2 ++
 src/tests/RequestBuilderTest.php | 53 ++++++++++++++++++++++++++------
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/src/src/RequestBuilder.php b/src/src/RequestBuilder.php
index 0ed0c74d..e503cba4 100644
--- a/src/src/RequestBuilder.php
+++ b/src/src/RequestBuilder.php
@@ -362,6 +362,8 @@ protected function wait_after_429( string $headers, int $max_wait = 60 ): int {
 		// And no longer than 60 seconds.
 		$retry_after = min( $max_wait, $retry_after );
 
+		$retry_after += rand( 0, 5 ); // Add a random number of seconds to avoid all clients retrying at the same time.
+
 		return $retry_after;
 	}
 
diff --git a/src/tests/RequestBuilderTest.php b/src/tests/RequestBuilderTest.php
index 9473cc8c..0c98a74b 100644
--- a/src/tests/RequestBuilderTest.php
+++ b/src/tests/RequestBuilderTest.php
@@ -2,6 +2,7 @@
 
 namespace QIT_CLI_Tests;
 
+use PHPUnit\Framework\AssertionFailedError;
 use QIT_CLI\RequestBuilder;
 use PHPUnit\Framework\TestCase;
 
@@ -20,10 +21,21 @@ public function wait_after_429( string $headers, int $max_wait = 60 ): int {
 		};
 	}
 
+	protected function assertRetryDelayWithinRange( $expected, $actual, $delta ) {
+		if ( $actual < $expected ) {
+			$this->fail( "Expected value is $expected, actual value is $actual, which is less than expected." );
+		} elseif ( $actual > $expected + $delta ) {
+			$this->fail( "Expected value is $expected, actual value is $actual, which is greater than expected + delta ($delta)." );
+		} else {
+			// If the actual value is within the acceptable range, explicitly assert true.
+			$this->assertTrue( true );
+		}
+	}
+
 	public function test_retry_after_seconds() {
 		$headers = "Retry-After: 59\r\nOther-Header: value";
 
-		$this->assertEquals( 59, $this->sut->wait_after_429( $headers ) );
+		$this->assertRetryDelayWithinRange( 59, $this->sut->wait_after_429( $headers ), 5 );
 	}
 
 	public function test_retry_after_http_date() {
@@ -34,35 +46,56 @@ public function test_retry_after_http_date() {
 		// Since time will pass between the creation of the date and this calculation,
 		// allow a small margin in the assertion
 		$expected_delay = $dateTime->getTimestamp() - time();
-		$this->assertEqualsWithDelta( $expected_delay, $this->sut->wait_after_429( $headers, 130 ), 5 );
+		$this->assertRetryDelayWithinRange( $expected_delay, $this->sut->wait_after_429( $headers, 130 ), 5 );
 	}
 
 	public function test_no_retry_after_header() {
 		$headers = "Other-Header: value";
 
-		$this->assertEquals( 5, $this->sut->wait_after_429( $headers ) );
+		$this->assertRetryDelayWithinRange( 5, $this->sut->wait_after_429( $headers ), 5 );
 	}
 
 	public function test_invalid_retry_after_header() {
 		$headers = "Retry-After: invalid\r\nOther-Header: value";
 
-		$this->assertEquals( 5, $this->sut->wait_after_429( $headers ) );
+		$this->assertRetryDelayWithinRange( 5, $this->sut->wait_after_429( $headers ), 5 );
 	}
 
 	public function test_exponential_backoff() {
 		$headers = "Retry-After: invalid\r\nOther-Header: value";
 
-		$this->assertEquals( 5, $this->sut->wait_after_429( $headers ) );
+		$this->assertRetryDelayWithinRange( 5, $this->sut->wait_after_429( $headers ), 5 );
 		// Mimick integration-level test.
 		$this->sut->retry_429 --;
-		$this->assertEquals( 10, $this->sut->wait_after_429( $headers ) );
+		$this->assertRetryDelayWithinRange( 10, $this->sut->wait_after_429( $headers ), 5 );
 		$this->sut->retry_429 --;
-		$this->assertEquals( 20, $this->sut->wait_after_429( $headers ) );
+		$this->assertRetryDelayWithinRange( 20, $this->sut->wait_after_429( $headers ), 5 );
 		$this->sut->retry_429 --;
-		$this->assertEquals( 40, $this->sut->wait_after_429( $headers ) );
+		$this->assertRetryDelayWithinRange( 40, $this->sut->wait_after_429( $headers ), 5 );
 		$this->sut->retry_429 --;
-		$this->assertEquals( 80, $this->sut->wait_after_429( $headers, 300 ) );
+		$this->assertRetryDelayWithinRange( 80, $this->sut->wait_after_429( $headers, 200 ), 5 );
 		$this->sut->retry_429 --;
-		$this->assertEquals( 160, $this->sut->wait_after_429( $headers, 300 ) );
+		$this->assertRetryDelayWithinRange( 160, $this->sut->wait_after_429( $headers, 200 ), 5 );
+	}
+
+	//
+	// Some tests for our custom assertion.
+	//
+	public function testExactValue() {
+		$this->assertRetryDelayWithinRange( 10, 10, 1 ); // Delta of 1
+	}
+
+	public function testWithinDelta() {
+		$this->assertRetryDelayWithinRange( 10, 11, 1 ); // Within delta of 1
+	}
+
+	public function testExceedsDelta() {
+		$this->expectException( AssertionFailedError::class );
+		$this->assertRetryDelayWithinRange( 10, 12, 1 ); // Exceeds delta of 1
+	}
+
+	public function testBelowExpected() {
+		$this->expectException( AssertionFailedError::class );
+		$this->assertRetryDelayWithinRange( 10, 9, 1 ); // Below expected
 	}
 }