fix(mcp): stop OAuth-broken connections from reporting Connected forever

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));

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}");
 

src/Netclaw.Daemon/Mcp/McpClientManager.cs

@@ -349,6 +349,12 @@ private async Task<bool> 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<AuthorizationResult?>(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(
         };
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    internal static Func<AuthorizationCallbackContext, CancellationToken, Task<AuthorizationResult?>> CreateAuthorizationCallbackHandler(
+        McpOAuthFlow? authorizationFlow)
+        => authorizationFlow is null
+            ? static (_, _) => Task.FromResult<AuthorizationResult?>(null)
+            : (context, ct) => authorizationFlow.IsTerminal
+                ? Task.FromResult<AuthorizationResult?>(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);
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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;