Skip to content

Classify clock-skew invalid_client as retriable, not a deleted registration #4694

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
canblmz1 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Classify clock-skew invalid_client as retriable, not a deleted registration #4694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
286984c
fix(listener): classify clock-skew invalid_client as retriable, not a…
canblmz1 Sep 9, 2026
0575e62
test(listener): mirror clock-skew invalid_client coverage in BrokerMe…
canblmz1 Sep 9, 2026
8afb5aa
fix(listener): drop the extra clock-skew terminal message per review
canblmz1 Sep 9, 2026
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
42 changes: 29 additions & 13 deletions src/Runner.Listener/BrokerMessageListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,22 +187,38 @@ public async Task<CreateSessionResult> 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
Comment on lines +190 to +194

@canblmz1 canblmz1 Sep 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fair catch - added mirrored coverage in BrokerMessageListenerL0 (CreateSession_ClockSkewInvalidClient_RetriesInsteadOfTerminating / CreateSession_InvalidClientWithoutClockSkew_StillTerminatesAsDeletedRegistration), same regression-proof method as the MessageListenerL0 tests (temporarily reverted BrokerMessageListener.cs alone and confirmed the skew test fails, then restored it). Full suite is 21/21 now.

// 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;
}
}
}

Expand Down
42 changes: 29 additions & 13 deletions src/Runner.Listener/MessageListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,22 +151,38 @@ public async Task<CreateSessionResult> 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;
}
}
}

Expand Down
137 changes: 137 additions & 0 deletions src/Test/L0/Listener/BrokerMessageListenerL0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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<ITerminal>();
tc.SetSingleton<ITerminal>(mockTerm.Object);

var rsaKeyManager = new Mock<IRSAKeyManager>();
rsaKeyManager.Setup(x => x.GetKey()).Returns(RSA.Create(2048));
tc.SetSingleton<IRSAKeyManager>(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<TaskAgentSession>(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<TaskAgentSession>(y => y != null),
tokenSource.Token), Times.Exactly(2));

mockTerm.Verify(x => x.WriteError(It.Is<string>(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<ITerminal>();
tc.SetSingleton<ITerminal>(mockTerm.Object);

var rsaKeyManager = new Mock<IRSAKeyManager>();
rsaKeyManager.Setup(x => x.GetKey()).Returns(RSA.Create(2048));
tc.SetSingleton<IRSAKeyManager>(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<TaskAgentSession>(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<TaskAgentSession>(y => y != null),
tokenSource.Token), Times.Once());

mockTerm.Verify(x => x.WriteError(It.Is<string>(s => s.Contains("registration has been deleted"))), Times.Once);
}
}

private TestHostContext CreateTestContext([CallerMemberName] String testName = "")
{
TestHostContext tc = new(this, testName);
Expand Down
Loading