Merge PR #1478: Fix unnecessary retries on repository existence checks

brianaj · brianaj · commit 2942ea9d56ed · 2025-12-12T19:25:25.000Z
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -1,2 +1,3 @@
 - Added validation to detect and return clear error messages when a URL is provided instead of a name for organization, repository, or enterprise arguments (e.g., `--github-org`, `--github-target-org`, `--source-repo`, `--github-target-enterprise`)
+- Fixed an issue where repository existence checks would incorrectly retry on expected responses (200/404/301), causing unnecessary delays during migrations. The check now only retries on transient server errors (5xx status codes) while responding immediately to deterministic states.
 - Added `--target-api-url` as an optional arg to the `add-team-to-repo` command
diff --git a/src/Octoshift/Services/GithubClient.cs b/src/Octoshift/Services/GithubClient.cs
@@ -45,7 +45,13 @@ public GithubClient(OctoLogger log, HttpClient httpClient, IVersionProvider vers
         }
     }
 
-    public virtual async Task<string> GetNonSuccessAsync(string url, HttpStatusCode status) => (await GetWithRetry(url, expectedStatus: status)).Content;
+    public virtual async Task<string> GetNonSuccessAsync(string url, HttpStatusCode status)
+    {
+        return (await _retryPolicy.HttpRetry(
+            async () => await SendAsync(HttpMethod.Get, url, expectedStatus: status),
+            ex => ex.StatusCode.HasValue && (int)ex.StatusCode.Value >= 500
+        )).Content;
+    }
 
     public virtual async Task<string> GetAsync(string url, Dictionary<string, string> customHeaders = null) => (await GetWithRetry(url, customHeaders)).Content;
 
diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs
@@ -1063,27 +1063,150 @@ public async Task GetNonSuccessAsync_Logs_The_GitHub_Request_Id_Header_Value()
     }
 
     [Fact]
-    public async Task GetNonSuccessAsync_Retries_On_Non_Success()
+    public async Task GetNonSuccessAsync_Does_Not_Retry_On_Expected_Status()
     {
         // Arrange
         var handlerMock = new Mock<HttpMessageHandler>();
         handlerMock
             .Protected()
-            .SetupSequence<Task<HttpResponseMessage>>(
+            .Setup<Task<HttpResponseMessage>>(
                 "SendAsync",
                 ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
                 ItExpr.IsAny<CancellationToken>())
-            .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.InternalServerError))
-            .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.Found, content: EXPECTED_RESPONSE_CONTENT));
+            .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.NotFound, content: "Not Found"));
 
         using var httpClient = new HttpClient(handlerMock.Object);
         var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN);
 
         // Act
-        var result = await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.Found);
+        var result = await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound);
+
+        // Assert
+        result.Should().Be("Not Found");
+        handlerMock.Protected().Verify(
+            "SendAsync",
+            Times.Once(),
+            ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+            ItExpr.IsAny<CancellationToken>());
+    }
+
+    [Fact]
+    public async Task GetNonSuccessAsync_Does_Not_Retry_On_Unexpected_Success_Status()
+    {
+        // Arrange
+        var handlerMock = new Mock<HttpMessageHandler>();
+        handlerMock
+            .Protected()
+            .Setup<Task<HttpResponseMessage>>(
+                "SendAsync",
+                ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+                ItExpr.IsAny<CancellationToken>())
+            .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.OK, content: "Success"));
+
+        using var httpClient = new HttpClient(handlerMock.Object);
+        var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN);
+
+        // Act & Assert - should throw immediately without retry
+        await FluentActions
+            .Invoking(async () => await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound))
+            .Should()
+            .ThrowExactlyAsync<HttpRequestException>()
+            .WithMessage("*OK*");
+
+        handlerMock.Protected().Verify(
+            "SendAsync",
+            Times.Once(),
+            ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+            ItExpr.IsAny<CancellationToken>());
+    }
+
+    [Fact]
+    public async Task GetNonSuccessAsync_Retries_On_Server_Error()
+    {
+        // Arrange
+        var handlerMock = new Mock<HttpMessageHandler>();
+        handlerMock
+            .Protected()
+            .SetupSequence<Task<HttpResponseMessage>>(
+                "SendAsync",
+                ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+                ItExpr.IsAny<CancellationToken>())
+            .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.InternalServerError, content: "Server Error"))
+            .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.NotFound, content: "Not Found"));
+
+        using var httpClient = new HttpClient(handlerMock.Object);
+        var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN);
+
+        // Act - should retry on 5xx and eventually succeed
+        var result = await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound);
 
         // Assert
-        result.Should().Be(EXPECTED_RESPONSE_CONTENT);
+        result.Should().Be("Not Found");
+        handlerMock.Protected().Verify(
+            "SendAsync",
+            Times.Exactly(2),
+            ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+            ItExpr.IsAny<CancellationToken>());
+    }
+
+    [Fact]
+    public async Task GetNonSuccessAsync_Does_Not_Retry_On_Client_Error()
+    {
+        // Arrange
+        var handlerMock = new Mock<HttpMessageHandler>();
+        handlerMock
+            .Protected()
+            .Setup<Task<HttpResponseMessage>>(
+                "SendAsync",
+                ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+                ItExpr.IsAny<CancellationToken>())
+            .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.BadRequest, content: "Bad Request"));
+
+        using var httpClient = new HttpClient(handlerMock.Object);
+        var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN);
+
+        // Act & Assert - should throw immediately without retry on 4xx
+        await FluentActions
+            .Invoking(async () => await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound))
+            .Should()
+            .ThrowExactlyAsync<HttpRequestException>()
+            .WithMessage("*BadRequest*");
+
+        handlerMock.Protected().Verify(
+            "SendAsync",
+            Times.Once(),
+            ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+            ItExpr.IsAny<CancellationToken>());
+    }
+
+    [Fact]
+    public async Task GetNonSuccessAsync_Does_Not_Retry_On_Redirect_Status()
+    {
+        // Arrange
+        var handlerMock = new Mock<HttpMessageHandler>();
+        handlerMock
+            .Protected()
+            .Setup<Task<HttpResponseMessage>>(
+                "SendAsync",
+                ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+                ItExpr.IsAny<CancellationToken>())
+            .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.MovedPermanently, content: "Moved"));
+
+        using var httpClient = new HttpClient(handlerMock.Object);
+        var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN);
+
+        // Act & Assert - should throw immediately without retry on redirect
+        await FluentActions
+            .Invoking(async () => await githubClient.GetNonSuccessAsync(URL, HttpStatusCode.NotFound))
+            .Should()
+            .ThrowExactlyAsync<HttpRequestException>()
+            .WithMessage("*MovedPermanently*");
+
+        handlerMock.Protected().Verify(
+            "SendAsync",
+            Times.Once(),
+            ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+            ItExpr.IsAny<CancellationToken>());
     }
 
     [Fact]

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,3 @@`
`1`	`1`	- Added validation to detect and return clear error messages when a URL is provided instead of a name for organization, repository, or enterprise arguments (e.g., `--github-org`, `--github-target-org`, `--source-repo`, `--github-target-enterprise`)
	`2`	`+- Fixed an issue where repository existence checks would incorrectly retry on expected responses (200/404/301), causing unnecessary delays during migrations. The check now only retries on transient server errors (5xx status codes) while responding immediately to deterministic states.`
`2`	`3`	- Added `--target-api-url` as an optional arg to the `add-team-to-repo` command
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,13 @@ public GithubClient(OctoLogger log, HttpClient httpClient, IVersionProvider vers`
`45`	`45`	`}`
`46`	`46`	`}`
`47`	`47`
`48`		`- public virtual async Task<string> GetNonSuccessAsync(string url, HttpStatusCode status) => (await GetWithRetry(url, expectedStatus: status)).Content;`
	`48`	`+ public virtual async Task<string> GetNonSuccessAsync(string url, HttpStatusCode status)`
	`49`	`+ {`
	`50`	`+ return (await _retryPolicy.HttpRetry(`
	`51`	`+ async () => await SendAsync(HttpMethod.Get, url, expectedStatus: status),`
	`52`	`+ ex => ex.StatusCode.HasValue && (int)ex.StatusCode.Value >= 500`
	`53`	`+ )).Content;`
	`54`	`+ }`
`49`	`55`
`50`	`56`	`public virtual async Task<string> GetAsync(string url, Dictionary<string, string> customHeaders = null) => (await GetWithRetry(url, customHeaders)).Content;`
`51`	`57`