diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpCatalogRefreshTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpCatalogRefreshTests.cs index 7745ceca01..dcc0bedd8f 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpCatalogRefreshTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpCatalogRefreshTests.cs @@ -233,6 +233,52 @@ public void CanonicalSchema_ChangesOnSchemaEdit() Assert.NotEqual(McpClientManager.CanonicalSchema(a), McpClientManager.CanonicalSchema(b)); } + [Fact] + public async Task AuthorizationFailureDuringRefresh_MarksAwaitingAuthAndStopsTheRefreshLoop() + { + var runtime = new McpClientManagerLifecycleTests.ControlledMcpClientRuntime(); + var plan = runtime.Enqueue(new McpClientManagerLifecycleTests.ClientPlan("tool_a")); + var time = new FakeTimeProvider(InitialTime); + await using var harness = CreateHarness(runtime, time); + await harness.Manager.StartAsync(TestContext.Current.CancellationToken); + + time.Advance(McpClientManager.CatalogRefreshInterval); + plan.ListFailure = new ModelContextProtocol.McpException( + "Failed to handle unauthorized response with 'Bearer' scheme. " + + "The AuthorizationCallbackHandler returned a null authorization result."); + Assert.False(await harness.Manager.TryRefreshCatalogAsync(ServerName, TestContext.Current.CancellationToken)); + + // The status must stop claiming a working connection, but the catalog stays + // visible so the operator can see which server needs reauthorization. + var status = harness.Manager.GetServerStatuses()[ServerName]; + Assert.Equal(McpConnectionState.AwaitingAuth, status.State); + Assert.Equal(1, status.ToolCount); + AssertPublishedTools(harness, "tool_a"); + + // The demoted status removes the server from the Connected refresh path: + // no further listing attempts while the token is known-dead. + time.Advance(McpClientManager.CatalogRefreshInterval); + Assert.False(await harness.Manager.TryRefreshCatalogAsync(ServerName, TestContext.Current.CancellationToken)); + Assert.Equal(1, plan.RefreshCount); + } + + [Fact] + public async Task StuckAuthorizationFlowDuringRefresh_MarksAwaitingAuth() + { + var runtime = new McpClientManagerLifecycleTests.ControlledMcpClientRuntime(); + var plan = runtime.Enqueue(new McpClientManagerLifecycleTests.ClientPlan("tool_a")); + var time = new FakeTimeProvider(InitialTime); + await using var harness = CreateHarness(runtime, time); + await harness.Manager.StartAsync(TestContext.Current.CancellationToken); + + time.Advance(McpClientManager.CatalogRefreshInterval); + plan.ListFailure = new McpOAuthAuthorizationInProgressException(ServerName); + Assert.False(await harness.Manager.TryRefreshCatalogAsync(ServerName, TestContext.Current.CancellationToken)); + + Assert.Equal(McpConnectionState.AwaitingAuth, harness.Manager.GetServerStatuses()[ServerName].State); + AssertPublishedTools(harness, "tool_a"); + } + private static void AssertPublishedTools(McpClientManagerLifecycleTests.ManagerHarness harness, params string[] expected) { Assert.Equal(expected, harness.Manager.GetToolNames(ServerName)); diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthFlowBrokerTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthFlowBrokerTests.cs index b882d4025c..4587c24f5e 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthFlowBrokerTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthFlowBrokerTests.cs @@ -285,6 +285,52 @@ public async Task TerminalFlowAllowsNewAttemptWithNewStateWhileKeepingOldStatus( Assert.Equal(403, broker.GetStatusByState("old-state").Error?.Status); } + [Fact] + public async Task TerminalFlowCallbackHandler_ReturnsNullInsteadOfAlreadyInProgress() + { + using var broker = CreateBroker(); + var flow = broker.StartOrJoin(ServerName).Flow; + flow.Fail(new McpErrorResponse("expired", "test")); + + // A client published by this flow keeps its delegate for the connection's whole + // life. When the access token expires hours later the SDK re-invokes it; the + // consumed flow must answer null (a clean failure the manager classifies) rather + // than throw "already in progress" on every retry forever. + var handler = McpClientManager.CreateAuthorizationCallbackHandler(flow); + var result = await handler( + new AuthorizationCallbackContext + { + AuthorizationUri = AuthorizationUrl("state-terminal"), + RedirectUri = RedirectUri, + }, + TestContext.Current.CancellationToken); + + Assert.Null(result); + } + + [Fact] + public async Task PendingFlowCallbackHandler_ForwardsToTheFlow() + { + using var broker = CreateBroker(); + var flow = broker.StartOrJoin(ServerName).Flow; + var handler = McpClientManager.CreateAuthorizationCallbackHandler(flow); + + var pending = handler( + new AuthorizationCallbackContext + { + AuthorizationUri = AuthorizationUrl("state-fwd"), + RedirectUri = RedirectUri, + }, + TestContext.Current.CancellationToken); + + await flow.WaitForAuthorizationRequestAsync(TestContext.Current.CancellationToken); + flow.DeliverAuthorizationResponse("code-1", "state-fwd", null); + + var result = await pending; + Assert.NotNull(result); + Assert.Equal("code-1", result!.Code); + } + private static Uri AuthorizationUrl(string state) => new($"https://auth.example/authorize?client_id=one&state={state}"); diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index 0b57d04ab3..f1efeb135a 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -349,6 +349,12 @@ private async Task RefreshCatalogCoreAsync( // Invariant: a failed refresh must never empty the catalog. Roll back the // throttle claim so the next 30s tick retries instead of waiting 5 minutes. lifecycle.RollbackCatalogRefreshClaim(previousRefreshMs); + if (IsAuthFailure(ex)) + { + MarkAwaitingAuthorization(lifecycle, current, ex); + return false; + } + _logger.LogWarning(ex, "MCP server '{Name}' catalog refresh failed; keeping generation {Generation} unchanged", current.Name.Value, @@ -1160,12 +1166,7 @@ private ClientOAuthOptions BuildOAuthOptions( Scopes = ParseScopes(entry.OAuthScope), TokenCache = cache, - // A background reconnect has no flow and therefore no operator at a browser. - // Returning null makes the SDK fail the connection instead of blocking on a - // redirect nobody will complete. - AuthorizationCallbackHandler = authorizationFlow is null - ? static (_, _) => Task.FromResult(null) - : authorizationFlow.HandleAuthorizationCallbackAsync, + AuthorizationCallbackHandler = CreateAuthorizationCallbackHandler(authorizationFlow), // DynamicClientRegistration is deliberately left unset. McpOAuthClientRegistrar // owns registration because the SDK hard-codes client_secret_post and cannot @@ -1174,6 +1175,23 @@ private ClientOAuthOptions BuildOAuthOptions( }; } + /// + /// Builds the SDK authorization callback delegate for a client. A background reconnect + /// has no flow and therefore no operator at a browser: returning null makes the SDK fail + /// the connection instead of blocking on a redirect nobody will complete. A flow-built + /// client gets a handler that forwards only while its flow is still pending: hours later, + /// when the access token expires and the SDK re-invokes the delegate, the consumed flow + /// must answer null (a loud "null authorization result" failure) instead of throwing + /// "authorization already in progress" on every retry forever. + /// + internal static Func> CreateAuthorizationCallbackHandler( + McpOAuthFlow? authorizationFlow) + => authorizationFlow is null + ? static (_, _) => Task.FromResult(null) + : (context, ct) => authorizationFlow.IsTerminal + ? Task.FromResult(null) + : authorizationFlow.HandleAuthorizationCallbackAsync(context, ct); + private Uri BuildRedirectUri() => new($"http://127.0.0.1:{_daemonConfig.Port}/api/mcp/oauth/callback"); @@ -1334,11 +1352,37 @@ private void EmitDisconnectedAlert(McpServerName serverName, string summary) return scopeString.Split(' ', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); } + /// + /// A Connected server whose catalog refresh fails on authentication cannot repair itself: + /// the SDK's recovery path needs an operator at a browser. Demote the status so health + /// reporting stops claiming a working connection and the 30s refresh loop stops retrying + /// a dead token, but keep the catalog visible — wiping it would hide which server needs + /// reauthorization. + /// + private void MarkAwaitingAuthorization( + McpServerLifecycle lifecycle, + McpServerSnapshot current, + Exception ex) + { + var status = CreateAwaitingAuthStatus(current.Name, _timeProvider.GetUtcNow()) + with { ToolCount = current.ToolFunctions.Count }; + lifecycle.Publish(current with { Status = status }); + _logger.LogWarning(ex, + "MCP server '{Name}' lost OAuth authorization during catalog refresh; marked AwaitingAuth", + current.Name.Value); + EmitAuthAlert(current.Name, + $"MCP server '{current.Name.Value}' lost OAuth authorization. Run: netclaw mcp auth {current.Name.Value}", + "authorization_expired"); + } + private static bool IsAuthFailure(Exception ex) { if (ex is HttpRequestException { StatusCode: HttpStatusCode.Unauthorized or HttpStatusCode.Forbidden }) return true; + if (ex is McpOAuthAuthorizationInProgressException) + return true; + if (IsAuthFailureMessage(ex.Message)) return true;