From c619dcbca837f498edaf7fcea3a58cb6ec21292f Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Thu, 5 Sep 2024 06:40:19 -0700 Subject: [PATCH 1/4] more handling --- src/Runner.Common/BrokerServer.cs | 2 +- src/Sdk/RSWebApi/Contracts/BrokerError.cs | 17 ++++++++++++++ src/Sdk/WebApi/WebApi/BrokerHttpClient.cs | 27 ++++++++++++++++------ src/Sdk/WebApi/WebApi/RawHttpClientBase.cs | 24 +++++++++++++++++++ 4 files changed, 62 insertions(+), 8 deletions(-) create mode 100644 src/Sdk/RSWebApi/Contracts/BrokerError.cs diff --git a/src/Runner.Common/BrokerServer.cs b/src/Runner.Common/BrokerServer.cs index 5e1311715c5..4c612e9615e 100644 --- a/src/Runner.Common/BrokerServer.cs +++ b/src/Runner.Common/BrokerServer.cs @@ -92,7 +92,7 @@ public Task ForceRefreshConnection(VssCredentials credentials) public bool ShouldRetryException(Exception ex) { - if (ex is AccessDeniedException ade && ade.ErrorCode == 1) + if (ex is AccessDeniedException ade) { return false; } diff --git a/src/Sdk/RSWebApi/Contracts/BrokerError.cs b/src/Sdk/RSWebApi/Contracts/BrokerError.cs new file mode 100644 index 00000000000..f3940f0338e --- /dev/null +++ b/src/Sdk/RSWebApi/Contracts/BrokerError.cs @@ -0,0 +1,17 @@ +using System.Runtime.Serialization; + +namespace GitHub.Actions.RunService.WebApi +{ + [DataContract] + public class BrokerError + { + [DataMember(Name = "source", EmitDefaultValue = false)] + public string Source { get; set; } + + [DataMember(Name = "code", EmitDefaultValue = false)] + public int Code { get; set; } + + [DataMember(Name = "errorMessage", EmitDefaultValue = false)] + public string Message { get; set; } + } +} diff --git a/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs b/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs index e9ad938fb9f..f67e82b5af4 100644 --- a/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs +++ b/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs @@ -103,6 +103,7 @@ public async Task GetRunnerMessageAsync( new HttpMethod("GET"), requestUri: requestUri, queryParameters: queryParams, + readErrorBody: true, cancellationToken: cancellationToken); if (result.IsSuccess) @@ -110,17 +111,23 @@ public async Task GetRunnerMessageAsync( return result.Value; } - // the only time we throw a `Forbidden` exception from Listener /messages is when the runner is - // disable_update and is too old to poll - if (result.StatusCode == HttpStatusCode.Forbidden) + if (TryParseErrorBody(result.ErrorBody, out BrokerError brokerError)) { - throw new AccessDeniedException($"{result.Error} Runner version v{runnerVersion} is deprecated and cannot receive messages.") + if (result.StatusCode == HttpStatusCode.Forbidden && brokerError.Code == 1) { - ErrorCode = 1 - }; + throw new AccessDeniedException($"{result.Error} Runner version v{runnerVersion} is deprecated and cannot receive messages.") + { + ErrorCode = 1 + }; + } + } + + if (result.StatusCode == HttpStatusCode.Forbidden) + { + throw new AccessDeniedException($"Request to {requestUri} failed with status code {result.StatusCode} and error message: {result.ErrorBody}"); } - throw new Exception($"Failed to get job message: {result.Error}"); + throw new Exception($"Request to {requestUri} failed with status code {result.StatusCode} and error message {result.Error}"); } public async Task CreateSessionAsync( @@ -172,5 +179,11 @@ public async Task DeleteSessionAsync( throw new Exception($"Failed to delete broker session: {result.Error}"); } + + protected static bool TryParseErrorBody(string errorBody, out BrokerError error) + { + var validator = new Func(e => e.Source != null && e.Source == "actions-broker-listener"); + return RawHttpClientBase.TryParseErrorBody(errorBody, validator, out error); + } } } diff --git a/src/Sdk/WebApi/WebApi/RawHttpClientBase.cs b/src/Sdk/WebApi/WebApi/RawHttpClientBase.cs index 23c51472487..1381c22cbb3 100644 --- a/src/Sdk/WebApi/WebApi/RawHttpClientBase.cs +++ b/src/Sdk/WebApi/WebApi/RawHttpClientBase.cs @@ -326,6 +326,30 @@ protected HttpRequestMessage CreateRequestMessage( return requestMessage; } + protected static bool TryParseErrorBody(string errorBody, Func validate, out T error) + { + if (!string.IsNullOrEmpty(errorBody)) + { + try + { + error = JsonUtility.FromString(errorBody); + + if (error == null) + { + return false; + } + + return validate(error); + } + catch (Exception) + { + } + } + + error = default(T); + return false; + } + /// /// The inner client. /// From 4740b3a73a494566369d903b85131c71eb233274 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Thu, 5 Sep 2024 11:37:18 -0700 Subject: [PATCH 2/4] more feedback --- src/Sdk/RSWebApi/Contracts/BrokerError.cs | 4 +- src/Sdk/RSWebApi/Contracts/BrokerErrorType.cs | 10 +++++ src/Sdk/WebApi/WebApi/BrokerHttpClient.cs | 41 ++++++++++++------- src/Sdk/WebApi/WebApi/RawHttpClientBase.cs | 24 ----------- 4 files changed, 39 insertions(+), 40 deletions(-) create mode 100644 src/Sdk/RSWebApi/Contracts/BrokerErrorType.cs diff --git a/src/Sdk/RSWebApi/Contracts/BrokerError.cs b/src/Sdk/RSWebApi/Contracts/BrokerError.cs index f3940f0338e..295fa5f26dd 100644 --- a/src/Sdk/RSWebApi/Contracts/BrokerError.cs +++ b/src/Sdk/RSWebApi/Contracts/BrokerError.cs @@ -8,8 +8,8 @@ public class BrokerError [DataMember(Name = "source", EmitDefaultValue = false)] public string Source { get; set; } - [DataMember(Name = "code", EmitDefaultValue = false)] - public int Code { get; set; } + [DataMember(Name = "errorType", EmitDefaultValue = false)] + public string Type { get; set; } [DataMember(Name = "errorMessage", EmitDefaultValue = false)] public string Message { get; set; } diff --git a/src/Sdk/RSWebApi/Contracts/BrokerErrorType.cs b/src/Sdk/RSWebApi/Contracts/BrokerErrorType.cs new file mode 100644 index 00000000000..3c4844b4a0d --- /dev/null +++ b/src/Sdk/RSWebApi/Contracts/BrokerErrorType.cs @@ -0,0 +1,10 @@ +using System.Runtime.Serialization; + +namespace GitHub.Actions.RunService.WebApi +{ + [DataContract] + public class BrokerErrorType + { + public const string RunnerVersionTooOld = "RunnerVersionTooOld"; + } +} diff --git a/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs b/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs index f67e82b5af4..23bef7da79e 100644 --- a/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs +++ b/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs @@ -113,21 +113,19 @@ public async Task GetRunnerMessageAsync( if (TryParseErrorBody(result.ErrorBody, out BrokerError brokerError)) { - if (result.StatusCode == HttpStatusCode.Forbidden && brokerError.Code == 1) + switch (brokerError.Type) { - throw new AccessDeniedException($"{result.Error} Runner version v{runnerVersion} is deprecated and cannot receive messages.") - { - ErrorCode = 1 - }; + case BrokerErrorType.RunnerVersionTooOld: + throw new AccessDeniedException(brokerError.Message) + { + ErrorCode = 1 + }; + default: + break; } } - if (result.StatusCode == HttpStatusCode.Forbidden) - { - throw new AccessDeniedException($"Request to {requestUri} failed with status code {result.StatusCode} and error message: {result.ErrorBody}"); - } - - throw new Exception($"Request to {requestUri} failed with status code {result.StatusCode} and error message {result.Error}"); + throw new Exception($"Request to {requestUri} failed with status: {result.StatusCode}. Error message {result.Error}"); } public async Task CreateSessionAsync( @@ -180,10 +178,25 @@ public async Task DeleteSessionAsync( throw new Exception($"Failed to delete broker session: {result.Error}"); } - protected static bool TryParseErrorBody(string errorBody, out BrokerError error) + private static bool TryParseErrorBody(string errorBody, out BrokerError error) { - var validator = new Func(e => e.Source != null && e.Source == "actions-broker-listener"); - return RawHttpClientBase.TryParseErrorBody(errorBody, validator, out error); + if (!string.IsNullOrEmpty(errorBody)) + { + try + { + error = JsonUtility.FromString(errorBody); + if (error?.Source == "actions-broker-listener") + { + return true; + } + } + catch (Exception) + { + } + } + + error = null; + return false; } } } diff --git a/src/Sdk/WebApi/WebApi/RawHttpClientBase.cs b/src/Sdk/WebApi/WebApi/RawHttpClientBase.cs index 1381c22cbb3..23c51472487 100644 --- a/src/Sdk/WebApi/WebApi/RawHttpClientBase.cs +++ b/src/Sdk/WebApi/WebApi/RawHttpClientBase.cs @@ -326,30 +326,6 @@ protected HttpRequestMessage CreateRequestMessage( return requestMessage; } - protected static bool TryParseErrorBody(string errorBody, Func validate, out T error) - { - if (!string.IsNullOrEmpty(errorBody)) - { - try - { - error = JsonUtility.FromString(errorBody); - - if (error == null) - { - return false; - } - - return validate(error); - } - catch (Exception) - { - } - } - - error = default(T); - return false; - } - /// /// The inner client. /// From 7e0795f7f06892d1627a0ba8ac4f9c7b48155c47 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Thu, 5 Sep 2024 11:41:27 -0700 Subject: [PATCH 3/4] add back compat --- src/Sdk/WebApi/WebApi/BrokerHttpClient.cs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs b/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs index 23bef7da79e..b02c47bc935 100644 --- a/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs +++ b/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs @@ -125,7 +125,16 @@ public async Task GetRunnerMessageAsync( } } - throw new Exception($"Request to {requestUri} failed with status: {result.StatusCode}. Error message {result.Error}"); + // temporary back compat + if (result.StatusCode == HttpStatusCode.Forbidden) + { + throw new AccessDeniedException($"{result.Error} Runner version v{runnerVersion} is deprecated and cannot receive messages.") + { + ErrorCode = 1 + }; + } + + throw new Exception($"Failed to get job message. Request to {requestUri} failed with status: {result.StatusCode}. Error message {result.Error}"); } public async Task CreateSessionAsync( From 0033ad8a6c549ad68e55e1f91345b7d1574733b4 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Thu, 5 Sep 2024 13:38:02 -0700 Subject: [PATCH 4/4] More feedback --- src/Sdk/RSWebApi/Contracts/BrokerError.cs | 7 +++++-- .../Contracts/{BrokerErrorType.cs => BrokerErrorKind.cs} | 2 +- src/Sdk/WebApi/WebApi/BrokerHttpClient.cs | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) rename src/Sdk/RSWebApi/Contracts/{BrokerErrorType.cs => BrokerErrorKind.cs} (85%) diff --git a/src/Sdk/RSWebApi/Contracts/BrokerError.cs b/src/Sdk/RSWebApi/Contracts/BrokerError.cs index 295fa5f26dd..c2e4bfa7b6b 100644 --- a/src/Sdk/RSWebApi/Contracts/BrokerError.cs +++ b/src/Sdk/RSWebApi/Contracts/BrokerError.cs @@ -8,8 +8,11 @@ public class BrokerError [DataMember(Name = "source", EmitDefaultValue = false)] public string Source { get; set; } - [DataMember(Name = "errorType", EmitDefaultValue = false)] - public string Type { get; set; } + [DataMember(Name = "errorKind", EmitDefaultValue = false)] + public string ErrorKind { get; set; } + + [DataMember(Name = "statusCode", EmitDefaultValue = false)] + public int StatusCode { get; set; } [DataMember(Name = "errorMessage", EmitDefaultValue = false)] public string Message { get; set; } diff --git a/src/Sdk/RSWebApi/Contracts/BrokerErrorType.cs b/src/Sdk/RSWebApi/Contracts/BrokerErrorKind.cs similarity index 85% rename from src/Sdk/RSWebApi/Contracts/BrokerErrorType.cs rename to src/Sdk/RSWebApi/Contracts/BrokerErrorKind.cs index 3c4844b4a0d..15c423017db 100644 --- a/src/Sdk/RSWebApi/Contracts/BrokerErrorType.cs +++ b/src/Sdk/RSWebApi/Contracts/BrokerErrorKind.cs @@ -3,7 +3,7 @@ namespace GitHub.Actions.RunService.WebApi { [DataContract] - public class BrokerErrorType + public class BrokerErrorKind { public const string RunnerVersionTooOld = "RunnerVersionTooOld"; } diff --git a/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs b/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs index b02c47bc935..8b67da2e5b9 100644 --- a/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs +++ b/src/Sdk/WebApi/WebApi/BrokerHttpClient.cs @@ -113,9 +113,9 @@ public async Task GetRunnerMessageAsync( if (TryParseErrorBody(result.ErrorBody, out BrokerError brokerError)) { - switch (brokerError.Type) + switch (brokerError.ErrorKind) { - case BrokerErrorType.RunnerVersionTooOld: + case BrokerErrorKind.RunnerVersionTooOld: throw new AccessDeniedException(brokerError.Message) { ErrorCode = 1