Skip to content

RetryPolicy: include service name in 401 Unauthorized error messages #1532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
offbyone wants to merge 1 commit into
base: main
Choose a base branch
from
+58 −26
Open

RetryPolicy: include service name in 401 Unauthorized error messages #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/Octoshift/Factories/GithubApiFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ GithubApi ISourceGithubApiFactory.Create(string apiUrl, string uploadsUrl, strin
apiUrl ??= DEFAULT_API_URL;
uploadsUrl ??= DEFAULT_UPLOADS_URL;
sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken();
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, _retryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub");
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, clientRetryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider);
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
}
Comment on lines +34 to 38
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clientRetryPolicy is created with a service name, but ArchiveUploader and GithubApi are still constructed with _retryPolicy. Any 401s thrown from _retryPolicy.Retry/HttpRetry inside GithubApi/ArchiveUploader will therefore not include the service name (and this also contradicts the PR goal of using per-service policies). Pass clientRetryPolicy to ArchiveUploader and GithubApi here so all GitHub operations use the tagged policy.

This issue also appears in the following locations of the same file:

  • line 45
  • line 56

Copilot uses AI. Check for mistakes.
Expand All @@ -41,7 +42,8 @@ GithubApi ISourceGithubApiFactory.CreateClientNoSsl(string apiUrl, string upload
apiUrl ??= DEFAULT_API_URL;
uploadsUrl ??= DEFAULT_UPLOADS_URL;
sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken();
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("NoSSL"), _versionProvider, _retryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub");
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("NoSSL"), _versionProvider, clientRetryPolicy, _dateTimeProvider, sourcePersonalAccessToken);
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider);
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
}
Expand All @@ -51,7 +53,8 @@ GithubApi ITargetGithubApiFactory.Create(string apiUrl, string uploadsUrl, strin
apiUrl ??= DEFAULT_API_URL;
uploadsUrl ??= DEFAULT_UPLOADS_URL;
targetPersonalAccessToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken();
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, _retryPolicy, _dateTimeProvider, targetPersonalAccessToken);
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("GitHub");
var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, clientRetryPolicy, _dateTimeProvider, targetPersonalAccessToken);
var multipartUploader = new ArchiveUploader(githubClient, uploadsUrl, _octoLogger, _retryPolicy, _environmentVariableProvider);
return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader);
}
Expand Down
35 changes: 30 additions & 5 deletions src/Octoshift/RetryPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,26 @@ namespace OctoshiftCLI
public class RetryPolicy
{
private readonly OctoLogger _log;
private readonly string _serviceName;
internal int _httpRetryInterval = 1000;
internal int _retryInterval = 4000;

public RetryPolicy(OctoLogger log)
public RetryPolicy(OctoLogger log, string serviceName = null)
{
_log = log;
_serviceName = serviceName;
}

/// <summary>
/// Returns a new RetryPolicy with the same configuration as this one, but tagged with a service name
/// for more actionable error messages (e.g. on 401 Unauthorized).
/// </summary>
public RetryPolicy WithServiceName(string serviceName) => new(_log, serviceName)
{
_httpRetryInterval = _httpRetryInterval,
_retryInterval = _retryInterval
};

public async Task<T> HttpRetry<T>(Func<Task<T>> func, Func<HttpRequestException, bool> filter)
{
var policy = Policy.Handle(filter)
Expand All @@ -26,7 +38,15 @@ public async Task<T> HttpRetry<T>(Func<Task<T>> func, Func<HttpRequestException,
_log?.LogVerbose($"Call failed with HTTP {((HttpRequestException)ex).StatusCode}, retrying...");
});

return await policy.ExecuteAsync(func);
try
{
return await policy.ExecuteAsync(func);
}
catch (HttpRequestException ex) when (ex.StatusCode == System.Net.HttpStatusCode.Unauthorized)
{
ThrowUnauthorizedException(ex);
throw; // unreachable, but required by compiler
}
}

public async Task<PolicyResult<T>> RetryOnResult<T>(Func<Task<T>> func, Func<T, bool> resultPredicate, string retryLogMessage = null)
Expand All @@ -53,15 +73,20 @@ private AsyncRetryPolicy CreateRetryPolicyForException<TException>() where TExce
_log?.LogVerbose($"[HTTP ERROR {(int?)httpEx.StatusCode}] {ex}");
if (httpEx.StatusCode == System.Net.HttpStatusCode.Unauthorized)
{
// We should not retry on an unathorized error; instead, log and bubble up the error
throw new OctoshiftCliException($"Unauthorized. Please check your token and try again", ex);
};
ThrowUnauthorizedException(httpEx);
}
}
else
{
_log?.LogVerbose(ex.ToString());
}
_log?.LogVerbose("Retrying...");
});

private void ThrowUnauthorizedException(HttpRequestException ex)
{
var serviceContext = _serviceName is not null ? $" ({_serviceName})" : "";
throw new OctoshiftCliException($"Unauthorized{serviceContext}. Please check your token and try again", ex);
}
}
}
6 changes: 3 additions & 3 deletions src/OctoshiftCLI.IntegrationTests/AdoToGithub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,14 @@ protected AdoToGithub(ITestOutputHelper output, string adoServerUrl = "https://d
_versionClient = new HttpClient();
var adoToken = Environment.GetEnvironmentVariable(adoPatEnvVar);
_adoHttpClient = new HttpClient();
var retryPolicy = new RetryPolicy(logger);
var retryPolicy = new RetryPolicy(logger, $"Azure DevOps ({adoPatEnvVar})");
var adoClient = new AdoClient(logger, _adoHttpClient, new VersionChecker(_versionClient, logger), retryPolicy, adoToken);
var adoApi = new AdoApi(adoClient, adoServerUrl, logger);

var githubToken = Environment.GetEnvironmentVariable("GHEC_PAT");
_githubHttpClient = new HttpClient();
var githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), githubToken);
var githubApi = new GithubApi(githubClient, "https://api.github.com", new RetryPolicy(logger), null);
var githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger, "GitHub (GHEC_PAT)"), new DateTimeProvider(), githubToken);
var githubApi = new GithubApi(githubClient, "https://api.github.com", new RetryPolicy(logger, "GitHub (GHEC_PAT)"), null);

Tokens = new Dictionary<string, string>
{
Expand Down
8 changes: 4 additions & 4 deletions src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ public BbsToGithub(ITestOutputHelper output)
_versionClient = new HttpClient();

_sourceBbsHttpClient = new HttpClient();
_sourceBbsClient = new BbsClient(_logger, _sourceBbsHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger), sourceBbsUsername, sourceBbsPassword);
_sourceBbsClient = new BbsClient(_logger, _sourceBbsHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger, "Bitbucket Server (BBS_USERNAME/BBS_PASSWORD)"), sourceBbsUsername, sourceBbsPassword);

_targetGithubHttpClient = new HttpClient();
_targetGithubClient = new GithubClient(_logger, _targetGithubHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger), new DateTimeProvider(), targetGithubToken);
var retryPolicy = new RetryPolicy(_logger);
_targetGithubClient = new GithubClient(_logger, _targetGithubHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger, "GitHub (GHEC_PAT)"), new DateTimeProvider(), targetGithubToken);
var retryPolicy = new RetryPolicy(_logger, "GitHub (GHEC_PAT)");
var environmentVariableProvider = new EnvironmentVariableProvider(_logger);
_archiveUploader = new ArchiveUploader(_targetGithubClient, UPLOADS_URL, _logger, retryPolicy, environmentVariableProvider);
_targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(_logger), _archiveUploader);
_targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(_logger, "GitHub (GHEC_PAT)"), _archiveUploader);

_blobServiceClient = new BlobServiceClient(_azureStorageConnectionString);

Expand Down
10 changes: 5 additions & 5 deletions src/OctoshiftCLI.IntegrationTests/GhesToGithub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,17 @@ public GhesToGithub(ITestOutputHelper output)
};

_versionClient = new HttpClient();
var retryPolicy = new RetryPolicy(logger);
var retryPolicy = new RetryPolicy(logger, "GHES (GHES_PAT)");
var environmentVariableProvider = new EnvironmentVariableProvider(logger);

_sourceGithubHttpClient = new HttpClient();
_sourceGithubClient = new GithubClient(logger, _sourceGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), sourceGithubToken);
_sourceGithubClient = new GithubClient(logger, _sourceGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger, "GHES (GHES_PAT)"), new DateTimeProvider(), sourceGithubToken);
_archiveUploader = new ArchiveUploader(_targetGithubClient, UPLOADS_URL, logger, retryPolicy, environmentVariableProvider);
_sourceGithubApi = new GithubApi(_sourceGithubClient, GHES_API_URL, new RetryPolicy(logger), _archiveUploader);
_sourceGithubApi = new GithubApi(_sourceGithubClient, GHES_API_URL, new RetryPolicy(logger, "GHES (GHES_PAT)"), _archiveUploader);

_targetGithubHttpClient = new HttpClient();
_targetGithubClient = new GithubClient(logger, _targetGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), targetGithubToken);
_targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(logger), _archiveUploader);
_targetGithubClient = new GithubClient(logger, _targetGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger, "GitHub (GHEC_PAT)"), new DateTimeProvider(), targetGithubToken);
_targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(logger, "GitHub (GHEC_PAT)"), _archiveUploader);

_blobServiceClient = new BlobServiceClient(_azureStorageConnectionString);

Expand Down
4 changes: 2 additions & 2 deletions src/OctoshiftCLI.IntegrationTests/GithubToGithub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ public GithubToGithub(ITestOutputHelper output)

_githubHttpClient = new HttpClient();
_versionClient = new HttpClient();
_githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), githubToken);
_githubApi = new GithubApi(_githubClient, "https://api.github.com", new RetryPolicy(logger), null);
_githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger, "GitHub (GHEC_PAT)"), new DateTimeProvider(), githubToken);
_githubApi = new GithubApi(_githubClient, "https://api.github.com", new RetryPolicy(logger, "GitHub (GHEC_PAT)"), null);

_helper = new TestHelper(_output, _githubApi, _githubClient);
}
Expand Down
3 changes: 2 additions & 1 deletion src/ado2gh/Factories/AdoApiFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ public virtual AdoApi Create(string adoServerUrl, string personalAccessToken)
{
adoServerUrl ??= DEFAULT_API_URL;
personalAccessToken ??= _environmentVariableProvider.AdoPersonalAccessToken();
var adoClient = new AdoClient(_octoLogger, _client, _versionProvider, _retryPolicy, personalAccessToken);
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("Azure DevOps");
var adoClient = new AdoClient(_octoLogger, _client, _versionProvider, clientRetryPolicy, personalAccessToken);
return new AdoApi(adoClient, adoServerUrl, _octoLogger);
}

Expand Down
3 changes: 2 additions & 1 deletion src/ado2gh/Factories/AdoPipelineTriggerServiceFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ public virtual AdoPipelineTriggerService Create(string adoServerUrl, string pers
{
adoServerUrl ??= DEFAULT_API_URL;
personalAccessToken ??= _environmentVariableProvider.AdoPersonalAccessToken();
var adoClient = new AdoClient(_octoLogger, _client, _versionProvider, _retryPolicy, personalAccessToken);
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("Azure DevOps");
var adoClient = new AdoClient(_octoLogger, _client, _versionProvider, clientRetryPolicy, personalAccessToken);
var adoApi = new AdoApi(adoClient, adoServerUrl, _octoLogger);
return new AdoPipelineTriggerService(adoApi, _octoLogger, adoServerUrl);
}
Expand Down
6 changes: 4 additions & 2 deletions src/bbs2gh/Factories/BbsApiFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,17 @@ public virtual BbsApi Create(string bbsServerUrl, string bbsUsername, string bbs

var httpClient = noSsl ? _clientFactory.CreateClient("NoSSL") : _clientFactory.CreateClient("Default");

var bbsClient = new BbsClient(_octoLogger, httpClient, _versionProvider, _retryPolicy, bbsUsername, bbsPassword);
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("Bitbucket Server");
var bbsClient = new BbsClient(_octoLogger, httpClient, _versionProvider, clientRetryPolicy, bbsUsername, bbsPassword);
return new BbsApi(bbsClient, bbsServerUrl, _octoLogger);
}

public virtual BbsApi CreateKerberos(string bbsServerUrl, bool noSsl = false)
{
var httpClient = noSsl ? _clientFactory.CreateClient("KerberosNoSSL") : _clientFactory.CreateClient("Kerberos");

var bbsClient = new BbsClient(_octoLogger, httpClient, _versionProvider, _retryPolicy);
var clientRetryPolicy = (_retryPolicy ?? new RetryPolicy(_octoLogger)).WithServiceName("Bitbucket Server");
var bbsClient = new BbsClient(_octoLogger, httpClient, _versionProvider, clientRetryPolicy);
return new BbsApi(bbsClient, bbsServerUrl, _octoLogger);
}
}
Loading