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

[canblmz1]

Summary

Fixes #4648. When a self-hosted runner's system clock hasn't synchronized yet at boot, CreateSessionAsync (both MessageListener and BrokerMessageListener) classifies the resulting OAuth invalid_client as a deleted registration, prints a misleading message, and returns CreateSessionResult.Failure unconditionally - which Runner.cs maps to ReturnCode.TerminatedError, so systemd never retries and the runner stays offline until manually restarted.

Root cause

invalid_client short-circuits to Failure before IsSessionCreationExceptionRetriable ever runs - in both listeners. That method already has a clock-skew retry path (checks for "Current server time is" in the exception message, retries every 30s for up to 30 minutes), but it's unreachable for this case because the invalid_client check above it returns first.

A clock-skewed token request also comes back as invalid_client, with "Current server time is ..." in the message - the exact sentinel IsSessionCreationExceptionRetriable already checks for. It just never gets there.

Fix

Minimal, no new retry policy: guard both invalid_client checks (the exception's own .Error, and the ValidateCredentialAsync fallback probe) in each listener on that same sentinel. When present, skip the deleted-registration classification and fall through to the existing clock-skew retry path instead of returning Failure. A genuine deleted registration (invalid_client without the sentinel) is unaffected - same message, same immediate Failure, unchanged.

Kept both listeners in sync; the fix is structurally identical in each (BrokerMessageListener keeps its existing !HostContext.AllowAuthMigration outer condition untouched).

Tests

Added to MessageListenerL0:

• CreateSession_ClockSkewInvalidClient_RetriesInsteadOfTerminating - invalid_client + clock-skew sentinel now retries (CreateAgentSessionAsync called twice: skew failure, then success) instead of terminating; no deleted-registration message is written.
• CreateSession_InvalidClientWithoutClockSkew_StillTerminatesAsDeletedRegistration - companion test proving a genuine deleted registration (no sentinel) still terminates immediately with exactly one attempt and the existing message - this fix must not weaken that path.

Verified both are real regression coverage, not just passing incidentally: temporarily reverted the source change (git stash on the two listener files only, tests kept) and reran - the skew test failed (Expected: Success, Actual: Failure) while the deleted-registration test still passed in both states. Restored the fix and reran the full MessageListenerL0 + BrokerMessageListenerL0 suite (19 tests) - all green.

dotnet test src/Test/Test.csproj -c Debug --filter "FullyQualifiedName~MessageListenerL0"

Compatibility with #4330

Checked #4330's full diff against this change before opening this PR: it modifies BrokerServer.ShouldRetryException (a different retry classifier, used for message polling, not session creation) and a separate RunnerNotFoundException classifier elsewhere in BrokerMessageListener.cs. It does not touch CreateSessionAsync or IsSessionCreationExceptionRetriable in either listener - confirmed by checking out its branch and diffing the resulting file directly. No behavioral overlap; at most a trivial rebase if both land, since both touch the same file in different regions.

Scope

src/Runner.Listener/MessageListener.cs, src/Runner.Listener/BrokerMessageListener.cs, src/Test/L0/Listener/MessageListenerL0.cs. No changes to systemd/NTP handling, no changes to ConnectivityAndDNSChecks (#4595), no new retry policy or configuration surface.

[Copilot]

🟡 Changes recommended

The new clock-skew branch adds noisy/duplicative terminal output and BrokerMessageListener’s behavior change lacks equivalent targeted L0 regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a self-hosted runner boot-time failure mode where OAuth invalid_client errors caused by system clock skew were incorrectly treated as a deleted runner registration, leading to an immediate CreateSessionResult.Failure and a non-retrying service exit.

Changes:

• Updates CreateSessionAsync in both MessageListener and BrokerMessageListener to bypass the “deleted registration” invalid_client short-circuit when the clock-skew sentinel ("Current server time is") is present, allowing the existing clock-skew retry path to run.
• Adds L0 regression tests to ensure clock-skew invalid_client retries instead of terminating, while true deleted-registration invalid_client still terminates immediately.

File summaries

File	Description
src/Runner.Listener/MessageListener.cs	Avoids misclassifying clock-skew invalid_client as deleted registration so existing retry logic can run.
src/Runner.Listener/BrokerMessageListener.cs	Mirrors the same clock-skew invalid_client handling change in the broker-based listener.
src/Test/L0/Listener/MessageListenerL0.cs	Adds regression coverage for clock-skew invalid_client retry vs. true deleted-registration termination.

Review details

Suppressed comments (1)

src/Runner.Listener/BrokerMessageListener.cs:201

• The clock-skew invalid_client path currently writes a new terminal error line on every retry, even though IsSessionCreationExceptionRetriable already prints a dedicated clock-skew message and the outer retry loop prints its own reconnect message. To avoid noisy/duplicative user output (and to ensure we only treat true invalid_client as clock-skew), make the condition explicitly "invalid_client" + sentinel (case-insensitive, null-safe) and rely on the existing clock-skew messaging.

                        if (vssOAuthEx.Message.Contains("Current server time is"))
                        {
                            _term.WriteError($"Failed to create a session because of a clock-skewed invalid_client error: {vssOAuthEx.Message}");
                            Trace.Info("invalid_client with a clock-skew signature detected; deferring to clock-skew retry classification instead of treating the registration as deleted.");
                        }

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[canblmz1]

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.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

🟢 Approval recommended

The change is narrowly scoped, keeps both listeners in sync, and is backed by targeted L0 regression tests covering both the fixed scenario and the unchanged terminal path.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite