diff --git a/src/Runner.Listener/BrokerMessageListener.cs b/src/Runner.Listener/BrokerMessageListener.cs index cfea82fd5ff..a77bcd11dbe 100644 --- a/src/Runner.Listener/BrokerMessageListener.cs +++ b/src/Runner.Listener/BrokerMessageListener.cs @@ -187,22 +187,38 @@ public async Task CreateSessionAsync(CancellationToken toke ex is VssOAuthTokenRequestException vssOAuthEx && _credsV2.Federated is VssOAuthCredential vssOAuthCred) { - // "invalid_client" means the runner registration has been deleted from the server. - if (string.Equals(vssOAuthEx.Error, "invalid_client", StringComparison.OrdinalIgnoreCase)) + // A clock-skewed token request also comes back as "invalid_client", but its + // message carries "Current server time is ..." - the same sentinel + // IsSessionCreationExceptionRetriable checks below. That is not a deleted + // registration; the request was rejected only because this machine's clock + // hasn't caught up yet. Skip the deleted-registration classification and let + // this fall through to the existing clock-skew retry path instead of + // terminating the runner. Nothing user-facing is logged here: that retry + // path already prints its own clock-skew message, and logging here too + // would duplicate it on every retry attempt. + if (vssOAuthEx.Message?.Contains("Current server time is") == true) { - _term.WriteError("Failed to create a session. The runner registration has been deleted from the server, please re-configure. Runner registrations are automatically deleted for runners that have not connected to the service recently."); - return CreateSessionResult.Failure; + Trace.Info("invalid_client with a clock-skew signature detected; deferring to clock-skew retry classification instead of treating the registration as deleted."); } - - // Check whether we get 401 because the runner registration already removed by the service. - // If the runner registration get deleted, we can't exchange oauth token. - Trace.Error("Test oauth app registration."); - var oauthTokenProvider = new VssOAuthTokenProvider(vssOAuthCred, new Uri(serverUrlV2)); - var authError = await oauthTokenProvider.ValidateCredentialAsync(token); - if (string.Equals(authError, "invalid_client", StringComparison.OrdinalIgnoreCase)) + else { - _term.WriteError("Failed to create a session. The runner registration has been deleted from the server, please re-configure. Runner registrations are automatically deleted for runners that have not connected to the service recently."); - return CreateSessionResult.Failure; + // "invalid_client" means the runner registration has been deleted from the server. + if (string.Equals(vssOAuthEx.Error, "invalid_client", StringComparison.OrdinalIgnoreCase)) + { + _term.WriteError("Failed to create a session. The runner registration has been deleted from the server, please re-configure. Runner registrations are automatically deleted for runners that have not connected to the service recently."); + return CreateSessionResult.Failure; + } + + // Check whether we get 401 because the runner registration already removed by the service. + // If the runner registration get deleted, we can't exchange oauth token. + Trace.Error("Test oauth app registration."); + var oauthTokenProvider = new VssOAuthTokenProvider(vssOAuthCred, new Uri(serverUrlV2)); + var authError = await oauthTokenProvider.ValidateCredentialAsync(token); + if (string.Equals(authError, "invalid_client", StringComparison.OrdinalIgnoreCase)) + { + _term.WriteError("Failed to create a session. The runner registration has been deleted from the server, please re-configure. Runner registrations are automatically deleted for runners that have not connected to the service recently."); + return CreateSessionResult.Failure; + } } } diff --git a/src/Runner.Listener/MessageListener.cs b/src/Runner.Listener/MessageListener.cs index ef06dd1af3c..6a6d2aaa49d 100644 --- a/src/Runner.Listener/MessageListener.cs +++ b/src/Runner.Listener/MessageListener.cs @@ -151,22 +151,38 @@ public async Task CreateSessionAsync(CancellationToken toke if (ex is VssOAuthTokenRequestException vssOAuthEx && _creds.Federated is VssOAuthCredential vssOAuthCred) { - // "invalid_client" means the runner registration has been deleted from the server. - if (string.Equals(vssOAuthEx.Error, "invalid_client", StringComparison.OrdinalIgnoreCase)) + // A clock-skewed token request also comes back as "invalid_client", but its + // message carries "Current server time is ..." - the same sentinel + // IsSessionCreationExceptionRetriable checks below. That is not a deleted + // registration; the request was rejected only because this machine's clock + // hasn't caught up yet. Skip the deleted-registration classification and let + // this fall through to the existing clock-skew retry path instead of + // terminating the runner. Nothing user-facing is logged here: that retry + // path already prints its own clock-skew message, and logging here too + // would duplicate it on every retry attempt. + if (vssOAuthEx.Message?.Contains("Current server time is") == true) { - _term.WriteError("Failed to create a session. The runner registration has been deleted from the server, please re-configure. Runner registrations are automatically deleted for runners that have not connected to the service recently."); - return CreateSessionResult.Failure; + Trace.Info("invalid_client with a clock-skew signature detected; deferring to clock-skew retry classification instead of treating the registration as deleted."); } - - // Check whether we get 401 because the runner registration already removed by the service. - // If the runner registration get deleted, we can't exchange oauth token. - Trace.Error("Test oauth app registration."); - var oauthTokenProvider = new VssOAuthTokenProvider(vssOAuthCred, new Uri(serverUrl)); - var authError = await oauthTokenProvider.ValidateCredentialAsync(token); - if (string.Equals(authError, "invalid_client", StringComparison.OrdinalIgnoreCase)) + else { - _term.WriteError("Failed to create a session. The runner registration has been deleted from the server, please re-configure. Runner registrations are automatically deleted for runners that have not connected to the service recently."); - return CreateSessionResult.Failure; + // "invalid_client" means the runner registration has been deleted from the server. + if (string.Equals(vssOAuthEx.Error, "invalid_client", StringComparison.OrdinalIgnoreCase)) + { + _term.WriteError("Failed to create a session. The runner registration has been deleted from the server, please re-configure. Runner registrations are automatically deleted for runners that have not connected to the service recently."); + return CreateSessionResult.Failure; + } + + // Check whether we get 401 because the runner registration already removed by the service. + // If the runner registration get deleted, we can't exchange oauth token. + Trace.Error("Test oauth app registration."); + var oauthTokenProvider = new VssOAuthTokenProvider(vssOAuthCred, new Uri(serverUrl)); + var authError = await oauthTokenProvider.ValidateCredentialAsync(token); + if (string.Equals(authError, "invalid_client", StringComparison.OrdinalIgnoreCase)) + { + _term.WriteError("Failed to create a session. The runner registration has been deleted from the server, please re-configure. Runner registrations are automatically deleted for runners that have not connected to the service recently."); + return CreateSessionResult.Failure; + } } } diff --git a/src/Test/L0/Listener/BrokerMessageListenerL0.cs b/src/Test/L0/Listener/BrokerMessageListenerL0.cs index 56ab7b3d066..217280ef513 100644 --- a/src/Test/L0/Listener/BrokerMessageListenerL0.cs +++ b/src/Test/L0/Listener/BrokerMessageListenerL0.cs @@ -1,12 +1,14 @@ using System; using System.IO; using System.Runtime.CompilerServices; +using System.Security.Cryptography; using System.Threading; using System.Threading.Tasks; using GitHub.DistributedTask.WebApi; using GitHub.Runner.Listener; using GitHub.Runner.Listener.Configuration; using GitHub.Services.Common; +using GitHub.Services.OAuth; using Moq; using Xunit; @@ -480,6 +482,141 @@ public async Task CreatesSessionWithProvidedSettings() } } + // Mirrors MessageListenerL0's coverage for the same fix (actions/runner#4648), for the + // broker listener - flagged in review as missing so a future change couldn't regress + // just the broker path without either suite catching it. + // + // Covers actions/runner#4648: an OAuth "invalid_client" caused purely by clock skew + // (the token request itself carries "Current server time is ..." in its message, the + // same sentinel IsSessionCreationExceptionRetriable already checks) must not be treated + // as a deleted registration. It must fall through to the existing clock-skew retry path + // instead of returning CreateSessionResult.Failure (which Runner.cs maps to + // ReturnCode.TerminatedError, so systemd never retries). + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Runner")] + public async Task CreateSession_ClockSkewInvalidClient_RetriesInsteadOfTerminating() + { + using (TestHostContext tc = CreateTestContext()) + using (var tokenSource = new CancellationTokenSource()) + { + Tracing trace = tc.GetTrace(); + + // Arrange. + var mockTerm = new Mock(); + tc.SetSingleton(mockTerm.Object); + + var rsaKeyManager = new Mock(); + rsaKeyManager.Setup(x => x.GetKey()).Returns(RSA.Create(2048)); + tc.SetSingleton(rsaKeyManager.Object); + + var oauth = new OAuthCredential(); + oauth.CredentialData = new CredentialData() { Scheme = Constants.Configuration.OAuth }; + oauth.CredentialData.Data.Add("clientId", "someClientId"); + oauth.CredentialData.Data.Add("authorizationUrl", "https://s.server"); + var federatedCreds = oauth.GetVssCredentials(tc, false); + // BrokerMessageListener.CreateSessionAsync loads _credsV2 via + // LoadCredentials(allowAuthUrlV2: true) - that is the credential object the + // invalid_client guard actually inspects. + _credMgr.Setup(x => x.LoadCredentials(true)).Returns(federatedCreds); + + var expectedSession = new TaskAgentSession(); + + // Reproduces the real message shape from the issue: a token-expiry check against + // the server's clock, surfaced by the service as an OAuth "invalid_client" error. + var skewException = new VssOAuthTokenRequestException( + "The token expired on 08/24/2026 19:15:44. Current server time is 08/25/2026 01:44:14.") + { + Error = "invalid_client", + }; + + _brokerServer + .SetupSequence(x => x.CreateSessionAsync( + It.Is(y => y != null), + tokenSource.Token)) + .Throws(skewException) + .Returns(Task.FromResult(expectedSession)); + + // Act. + BrokerMessageListener listener = new(); + listener.Initialize(tc); + + CreateSessionResult result = await listener.CreateSessionAsync(tokenSource.Token); + trace.Info("result: {0}", result); + + // Assert: the clock-skew invalid_client did not terminate the call - it fell + // through to the existing clock-skew retry path, and the next attempt (clock now + // caught up) succeeded. + Assert.Equal(CreateSessionResult.Success, result); + _brokerServer + .Verify(x => x.CreateSessionAsync( + It.Is(y => y != null), + tokenSource.Token), Times.Exactly(2)); + + mockTerm.Verify(x => x.WriteError(It.Is(s => s.Contains("registration has been deleted"))), Times.Never); + } + } + + // Companion to the test above: a genuine deleted-registration invalid_client (no + // clock-skew sentinel in the message) must keep terminating immediately, exactly as + // before. This fix must not weaken true deleted-registration handling. + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Runner")] + public async Task CreateSession_InvalidClientWithoutClockSkew_StillTerminatesAsDeletedRegistration() + { + using (TestHostContext tc = CreateTestContext()) + using (var tokenSource = new CancellationTokenSource()) + { + Tracing trace = tc.GetTrace(); + + // Arrange. + var mockTerm = new Mock(); + tc.SetSingleton(mockTerm.Object); + + var rsaKeyManager = new Mock(); + rsaKeyManager.Setup(x => x.GetKey()).Returns(RSA.Create(2048)); + tc.SetSingleton(rsaKeyManager.Object); + + var oauth = new OAuthCredential(); + oauth.CredentialData = new CredentialData() { Scheme = Constants.Configuration.OAuth }; + oauth.CredentialData.Data.Add("clientId", "someClientId"); + oauth.CredentialData.Data.Add("authorizationUrl", "https://s.server"); + var federatedCreds = oauth.GetVssCredentials(tc, false); + _credMgr.Setup(x => x.LoadCredentials(true)).Returns(federatedCreds); + + // A genuine deleted-registration response: invalid_client with no clock-skew + // sentinel anywhere in the message. + var deletedException = new VssOAuthTokenRequestException("Client authentication failed.") + { + Error = "invalid_client", + }; + + _brokerServer + .Setup(x => x.CreateSessionAsync( + It.Is(y => y != null), + tokenSource.Token)) + .Throws(deletedException); + + // Act. + BrokerMessageListener listener = new(); + listener.Initialize(tc); + + CreateSessionResult result = await listener.CreateSessionAsync(tokenSource.Token); + trace.Info("result: {0}", result); + + // Assert: still terminates immediately as a deleted registration - exactly one + // attempt, no retry. + Assert.Equal(CreateSessionResult.Failure, result); + _brokerServer + .Verify(x => x.CreateSessionAsync( + It.Is(y => y != null), + tokenSource.Token), Times.Once()); + + mockTerm.Verify(x => x.WriteError(It.Is(s => s.Contains("registration has been deleted"))), Times.Once); + } + } + private TestHostContext CreateTestContext([CallerMemberName] String testName = "") { TestHostContext tc = new(this, testName); diff --git a/src/Test/L0/Listener/MessageListenerL0.cs b/src/Test/L0/Listener/MessageListenerL0.cs index 80792539be9..1d5aeed934f 100644 --- a/src/Test/L0/Listener/MessageListenerL0.cs +++ b/src/Test/L0/Listener/MessageListenerL0.cs @@ -10,6 +10,7 @@ using GitHub.Runner.Listener; using GitHub.Runner.Listener.Configuration; using GitHub.Services.Common; +using GitHub.Services.OAuth; using GitHub.Services.WebApi; using Moq; using Xunit; @@ -737,5 +738,137 @@ public async Task GetNextMessageWithBrokerMigration_EnableAuthMigration() Assert.True(tc.AllowAuthMigration); } } + + // Covers actions/runner#4648: an OAuth "invalid_client" caused purely by clock skew + // (the token request itself carries "Current server time is ..." in its message, the + // same sentinel IsSessionCreationExceptionRetriable already checks) must not be treated + // as a deleted registration. It must fall through to the existing clock-skew retry path + // instead of returning CreateSessionResult.Failure (which Runner.cs maps to + // ReturnCode.TerminatedError, so systemd never retries). + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Runner")] + public async Task CreateSession_ClockSkewInvalidClient_RetriesInsteadOfTerminating() + { + using (TestHostContext tc = CreateTestContext()) + using (var tokenSource = new CancellationTokenSource()) + { + Tracing trace = tc.GetTrace(); + + // Arrange. + var mockTerm = new Mock(); + tc.SetSingleton(mockTerm.Object); + + var rsaKeyManager = new Mock(); + rsaKeyManager.Setup(x => x.GetKey()).Returns(RSA.Create(2048)); + tc.SetSingleton(rsaKeyManager.Object); + + var oauth = new OAuthCredential(); + oauth.CredentialData = new CredentialData() { Scheme = Constants.Configuration.OAuth }; + oauth.CredentialData.Data.Add("clientId", "someClientId"); + oauth.CredentialData.Data.Add("authorizationUrl", "https://s.server"); + var federatedCreds = oauth.GetVssCredentials(tc, false); + _credMgr.Setup(x => x.LoadCredentials(It.IsAny())).Returns(federatedCreds); + + var expectedSession = new TaskAgentSession(); + + // Reproduces the real message shape from the issue: a token-expiry check against + // the server's clock, surfaced by the service as an OAuth "invalid_client" error. + var skewException = new VssOAuthTokenRequestException( + "The token expired on 08/24/2026 19:15:44. Current server time is 08/25/2026 01:44:14.") + { + Error = "invalid_client", + }; + + _runnerServer + .SetupSequence(x => x.CreateAgentSessionAsync( + _settings.PoolId, + It.Is(y => y != null), + tokenSource.Token)) + .Throws(skewException) + .Returns(Task.FromResult(expectedSession)); + + // Act. + MessageListener listener = new(); + listener.Initialize(tc); + + CreateSessionResult result = await listener.CreateSessionAsync(tokenSource.Token); + trace.Info("result: {0}", result); + + // Assert: the clock-skew invalid_client did not terminate the call - it fell + // through to the existing clock-skew retry path, and the next attempt (clock now + // caught up) succeeded. + Assert.Equal(CreateSessionResult.Success, result); + _runnerServer + .Verify(x => x.CreateAgentSessionAsync( + _settings.PoolId, + It.Is(y => y != null), + tokenSource.Token), Times.Exactly(2)); + + mockTerm.Verify(x => x.WriteError(It.Is(s => s.Contains("registration has been deleted"))), Times.Never); + } + } + + // Companion to the test above: a genuine deleted-registration invalid_client (no + // clock-skew sentinel in the message) must keep terminating immediately, exactly as + // before. This fix must not weaken true deleted-registration handling. + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Runner")] + public async Task CreateSession_InvalidClientWithoutClockSkew_StillTerminatesAsDeletedRegistration() + { + using (TestHostContext tc = CreateTestContext()) + using (var tokenSource = new CancellationTokenSource()) + { + Tracing trace = tc.GetTrace(); + + // Arrange. + var mockTerm = new Mock(); + tc.SetSingleton(mockTerm.Object); + + var rsaKeyManager = new Mock(); + rsaKeyManager.Setup(x => x.GetKey()).Returns(RSA.Create(2048)); + tc.SetSingleton(rsaKeyManager.Object); + + var oauth = new OAuthCredential(); + oauth.CredentialData = new CredentialData() { Scheme = Constants.Configuration.OAuth }; + oauth.CredentialData.Data.Add("clientId", "someClientId"); + oauth.CredentialData.Data.Add("authorizationUrl", "https://s.server"); + var federatedCreds = oauth.GetVssCredentials(tc, false); + _credMgr.Setup(x => x.LoadCredentials(It.IsAny())).Returns(federatedCreds); + + // A genuine deleted-registration response: invalid_client with no clock-skew + // sentinel anywhere in the message. + var deletedException = new VssOAuthTokenRequestException("Client authentication failed.") + { + Error = "invalid_client", + }; + + _runnerServer + .Setup(x => x.CreateAgentSessionAsync( + _settings.PoolId, + It.Is(y => y != null), + tokenSource.Token)) + .Throws(deletedException); + + // Act. + MessageListener listener = new(); + listener.Initialize(tc); + + CreateSessionResult result = await listener.CreateSessionAsync(tokenSource.Token); + trace.Info("result: {0}", result); + + // Assert: still terminates immediately as a deleted registration - exactly one + // attempt, no retry. + Assert.Equal(CreateSessionResult.Failure, result); + _runnerServer + .Verify(x => x.CreateAgentSessionAsync( + _settings.PoolId, + It.Is(y => y != null), + tokenSource.Token), Times.Once()); + + mockTerm.Verify(x => x.WriteError(It.Is(s => s.Contains("registration has been deleted"))), Times.Once); + } + } } }