diff --git a/Darling/Darling.Tests/DarlingMcpServerAdminToolsTests.cs b/Darling/Darling.Tests/DarlingMcpServerAdminToolsTests.cs index be1ba49003..d30f12f3a2 100644 --- a/Darling/Darling.Tests/DarlingMcpServerAdminToolsTests.cs +++ b/Darling/Darling.Tests/DarlingMcpServerAdminToolsTests.cs @@ -192,8 +192,9 @@ private static string[] AllAddStatuses() => typeof(DarlingMcpServerAdminTools.Ad public void EveryAddStatus_HasExactlyOneSummaryCounter_AndNoCounterIsUnnamed() { var statuses = AllAddStatuses(); - Assert.Equal(5, statuses.Length); + Assert.Equal(6, statuses.Length); Assert.Contains("collides", statuses); + Assert.Contains("not_saved", statuses); var mapped = DarlingMcpServerAdminTools.CounterOfStatus.Keys.OrderBy(s => s, StringComparer.Ordinal).ToArray(); Assert.Equal(statuses, mapped); @@ -205,6 +206,7 @@ public void EveryAddStatus_HasExactlyOneSummaryCounter_AndNoCounterIsUnnamed() must not be retried as sent, which is exactly what a failed entry invites. */ Assert.Equal("collided", DarlingMcpServerAdminTools.CounterOfStatus["collides"]); Assert.Equal("skipped", DarlingMcpServerAdminTools.CounterOfStatus["duplicate"]); + Assert.Equal("failed", DarlingMcpServerAdminTools.CounterOfStatus["not_saved"]); } /// @@ -295,7 +297,7 @@ public void AddServersDescription_NamesEveryCounter_TheSumRule_AndTheCollidesSta var method = ToolMethods().Single(m => m.GetCustomAttribute()!.Name == "add_servers"); var description = method.GetCustomAttribute()!.Description; - foreach (var token in new[] { "requested:N", "added:N", "skipped:N", "collided:N", "failed:N", "SUM", "\"collides\"" }) + foreach (var token in new[] { "requested:N", "added:N", "skipped:N", "collided:N", "failed:N", "SUM", "\"collides\"", "\"not_saved\"" }) { Assert.Contains(token, description, StringComparison.Ordinal); } @@ -501,9 +503,12 @@ public void ParseRequest_ValidWindowsEntry_BuildsIntegratedProbeConfig_NoSecret( [Fact] public void ParseRequest_SqlEntry_CarriesPlaintextForProbe_AndAllExposedOptions() { + /* A literal password is only accepted where it can be encrypted, so the platform is named: this test is + about what the probe config carries, not about which platform it runs on. */ var (entries, invalid, wholeError) = DarlingMcpServerAdminTools.ParseRequest( "[{\"host\":\"sql02\",\"display_name\":\"Prod\",\"database\":\"AppDb\",\"auth\":\"SQL\",\"username\":\"monitor\"," + - "\"password\":\"p@ss\",\"encrypt_mode\":\"Strict\",\"trust_server_certificate\":true,\"read_only_intent\":true}]"); + "\"password\":\"p@ss\",\"encrypt_mode\":\"Strict\",\"trust_server_certificate\":true,\"read_only_intent\":true}]", + isWindows: true); Assert.Null(wholeError); Assert.Empty(invalid); @@ -527,7 +532,8 @@ public void ParseRequest_ServicePrincipalEntry_CarriesClientIdAndSecret_ForProbe secret in password, DPAPI-encrypted after a successful probe exactly like a SQL password. */ var (entries, invalid, wholeError) = DarlingMcpServerAdminTools.ParseRequest( "[{\"host\":\"azuredb.database.windows.net\",\"database\":\"AppDb\",\"auth\":\"ServicePrincipal\"," + - "\"username\":\"11111111-2222-3333-4444-555555555555\",\"password\":\"the-client-secret\"}]"); + "\"username\":\"11111111-2222-3333-4444-555555555555\",\"password\":\"the-client-secret\"}]", + isWindows: true); Assert.Null(wholeError); Assert.Empty(invalid); @@ -597,6 +603,268 @@ public void PartitionDuplicates_EntryMatchingAnExistingServer_IsSkipped() Assert.Empty(ready); Assert.Single(duplicates); } + + /* ---------------- #4734: every entry gets a result, and a save that wrote nothing is never "added" ---------------- */ + + private static readonly ConnectionProbeResult HealthyProbeResult = new( + Success: true, MajorVersion: 15, EngineEdition: 3, EngineEditionDescription: "Enterprise", + IsAzureSqlDb: false, IsAzureManagedInstance: false, IsAwsRds: false, HasMsdbAccess: true, Error: null); + + /// Two hosts whose storage keys hash to the SAME server_id (found by search; the test asserts it, so + /// a change to the hash says so instead of quietly voiding the collision). + private const string HolderHost = "sql9jocsv"; + private const string CollidingHost = "sqlsvvqew"; + + private static string StorageKeyOfHost(string host) => + DarlingMcpServerAdminTools.ParseRequest($"[{{\"host\":\"{host}\"}}]").Entries.Single().StorageKey; + + /// A definitions table that keeps the parts of the real one these tests depend on: server_id is the + /// hash of the storage key, a write to a taken id changes 0 rows (ON CONFLICT (server_id) DO NOTHING), and + /// a chosen write can throw. + private sealed class FakeDefinitions : DarlingMcpServerAdminTools.IServerDefinitions + { + private readonly Dictionary _rows = new(); + private readonly List _visibleToTheGate; + private readonly int _faultOnInsert; + private readonly bool _swallowEveryInsert; + + public FakeDefinitions( + string[]? existing = null, string[]? savedByAConcurrentWriter = null, int faultOnInsert = 0, bool swallowEveryInsert = false) + { + _visibleToTheGate = (existing ?? Array.Empty()).ToList(); + foreach (var key in _visibleToTheGate.Concat(savedByAConcurrentWriter ?? Array.Empty())) + { + _rows[ServerIdHelper.GetDeterministicHashCode(key)] = key; + } + + _faultOnInsert = faultOnInsert; + _swallowEveryInsert = swallowEveryInsert; + } + + /// The hosts of the entries that were really saved, in order. + public List Saved { get; } = new(); + + /// How many writes were attempted. + public int InsertAttempts { get; private set; } + + public Task> LoadStorageKeysAsync(CancellationToken cancellationToken) => + Task.FromResult(_visibleToTheGate.ToList()); + + public Task InsertAsync( + DarlingMcpServerAdminTools.ParsedServerEntry entry, string? encryptedPassword, CancellationToken cancellationToken) + { + InsertAttempts++; + if (InsertAttempts == _faultOnInsert) + { + throw new InvalidOperationException("57P01: terminating connection due to administrator command"); + } + + if (_swallowEveryInsert || !_rows.TryAdd(ServerIdHelper.GetDeterministicHashCode(entry.StorageKey), entry.StorageKey)) + { + return Task.FromResult(0); + } + + Saved.Add(entry.ProbeConfig.Host); + return Task.FromResult(1); + } + + public Task ReadStorageKeyAsync(int serverId, CancellationToken cancellationToken) => + Task.FromResult(_rows.TryGetValue(serverId, out var key) ? key : null); + } + + /// Entry 1 saves, entry 2's save throws, entry 3 is never reached: before this, the catch around the whole + /// loop threw away the list that recorded entry 1 as added, so the caller was told the call failed for a server + /// that was saved and collected, and a retry then reported it as a duplicate. Every entry now gets a result, + /// the summary counts them, and nothing after the fault is probed (each probe waits out a connect timeout). + [Theory] + [InlineData(1, 0)] + [InlineData(2, 1)] + [InlineData(3, 2)] + public async Task AddServers_ASaveThatFaults_ReportsEveryEntry_AndProbesNothingAfterTheFault(int faultOnEntry, int expectedAdded) + { + var hosts = new[] { "one", "two", "three" }; + var definitions = new FakeDefinitions(faultOnInsert: faultOnEntry); + var probed = new List(); + DarlingMcpServerAdminTools.ServerProbe probe = (server, _) => + { + probed.Add(server.Host); + return Task.FromResult(HealthyProbeResult); + }; + + var result = await DarlingMcpServerAdminTools.AddServersAsync( + definitions, "[{\"host\":\"one\"},{\"host\":\"two\"},{\"host\":\"three\"}]", probe, CancellationToken.None); + + using var doc = JsonDocument.Parse(result); + var root = doc.RootElement; + Assert.True(root.TryGetProperty("results", out var results), + "the call answered with one error instead of a result for each entry: " + result); + Assert.Equal(3, root.GetProperty("requested").GetInt32()); + Assert.Equal(expectedAdded, root.GetProperty("added").GetInt32()); + Assert.Equal(3 - expectedAdded, root.GetProperty("failed").GetInt32()); + Assert.Equal(0, root.GetProperty("skipped").GetInt32()); + Assert.Equal(0, root.GetProperty("collided").GetInt32()); + + var statuses = results.EnumerateArray().Select(r => r.GetProperty("status").GetString()).ToArray(); + Assert.Equal(Enumerable.Range(0, 3).Select(i => i < expectedAdded ? "added" : "not_saved").ToArray(), statuses); + + /* The entry whose save faulted WAS probed (that is how it reached the save); nothing after it was. */ + Assert.Equal(hosts.Take(faultOnEntry).ToArray(), probed); + Assert.Equal(hosts.Take(expectedAdded).ToArray(), definitions.Saved); + Assert.Equal(faultOnEntry, definitions.InsertAttempts); + + Assert.Contains("57P01", results[faultOnEntry - 1].GetProperty("detail").GetString(), StringComparison.Ordinal); + for (var i = faultOnEntry; i < 3; i++) + { + Assert.Contains("earlier entry", results[i].GetProperty("detail").GetString(), StringComparison.Ordinal); + } + } + + [Fact] + public void LiteralSecretRefusal_OffWindows_RefusesALiteral_AndNamesTheReferenceForms() + { + var password = DarlingMcpServerAdminTools.LiteralSecretRefusal("p@ss", isWindows: false, isServicePrincipal: false); + Assert.NotNull(password); + Assert.Contains("password", password, StringComparison.Ordinal); + Assert.Contains("env:NAME", password, StringComparison.Ordinal); + Assert.Contains("file:/run/secrets/", password, StringComparison.Ordinal); + Assert.DoesNotContain("p@ss", password, StringComparison.Ordinal); + + var clientSecret = DarlingMcpServerAdminTools.LiteralSecretRefusal("s3cret", isWindows: false, isServicePrincipal: true); + Assert.NotNull(clientSecret); + Assert.Contains("client secret", clientSecret, StringComparison.Ordinal); + Assert.DoesNotContain("s3cret", clientSecret, StringComparison.Ordinal); + } + + [Theory] + [InlineData("env:SQL_PW")] + [InlineData("file:/run/secrets/sql_password")] + public void LiteralSecretRefusal_AReferenceIsAcceptedOnEveryPlatform(string reference) + { + Assert.Null(DarlingMcpServerAdminTools.LiteralSecretRefusal(reference, isWindows: false, isServicePrincipal: false)); + Assert.Null(DarlingMcpServerAdminTools.LiteralSecretRefusal(reference, isWindows: true, isServicePrincipal: false)); + } + + [Fact] + public void LiteralSecretRefusal_OnWindows_AcceptsALiteral() + { + Assert.Null(DarlingMcpServerAdminTools.LiteralSecretRefusal("p@ss", isWindows: true, isServicePrincipal: false)); + } + + private const string LiteralSqlPasswordRequest = + "[{\"host\":\"sql01\",\"auth\":\"SQL\",\"username\":\"monitor\",\"password\":\"p@ss\"}]"; + + [Fact] + public void ParseRequest_ALiteralPassword_IsInvalidOffWindows_AndAcceptedOnWindows() + { + var (offEntries, offInvalid, offWholeError) = DarlingMcpServerAdminTools.ParseRequest(LiteralSqlPasswordRequest, isWindows: false); + Assert.Null(offWholeError); + Assert.Empty(offEntries); + var refused = Assert.Single(offInvalid); + Assert.Equal("invalid", refused.Status); + Assert.Equal("sql01", refused.Server); + Assert.Contains("file:/run/secrets/", refused.Detail, StringComparison.Ordinal); + + var (onEntries, onInvalid, _) = DarlingMcpServerAdminTools.ParseRequest(LiteralSqlPasswordRequest, isWindows: true); + Assert.Single(onEntries); + Assert.Empty(onInvalid); + } + + /// The overload every caller uses reads the platform it runs on, so on a non-Windows host it is the + /// request itself that refuses the literal, before any probe or write. + [Fact] + public void ParseRequest_ALiteralPassword_FollowsThePlatformTheTestRunsOn() + { + var (entries, invalid, _) = DarlingMcpServerAdminTools.ParseRequest(LiteralSqlPasswordRequest); + + if (OperatingSystem.IsWindows()) + { + Assert.Single(entries); + Assert.Empty(invalid); + } + else + { + Assert.Empty(entries); + Assert.Equal("invalid", Assert.Single(invalid).Status); + } + } + + [Fact] + public void ParseRequest_OffWindows_RefusesOnlyTheLiteralEntries_AndKeepsTheReferencesAndTheRest() + { + var (entries, invalid, _) = DarlingMcpServerAdminTools.ParseRequest( + "[{\"host\":\"win1\"}," + + "{\"host\":\"sql1\",\"auth\":\"SQL\",\"username\":\"u\",\"password\":\"literal\"}," + + "{\"host\":\"sql2\",\"auth\":\"SQL\",\"username\":\"u\",\"password\":\"file:/run/secrets/sql_password\"}," + + "{\"host\":\"az1\",\"auth\":\"ServicePrincipal\",\"username\":\"app\",\"password\":\"literal-secret\"}," + + "{\"host\":\"az2\",\"auth\":\"ServicePrincipal\",\"username\":\"app\",\"password\":\"env:CLIENT_SECRET\"}]", + isWindows: false); + + Assert.Equal(new[] { 0, 2, 4 }, entries.Select(e => e.Order).ToArray()); + Assert.Equal(new[] { 1, 3 }, invalid.Select(r => r.Order).ToArray()); + Assert.All(invalid, r => Assert.Equal("invalid", r.Status)); + Assert.Contains("password", invalid[0].Detail, StringComparison.Ordinal); + Assert.Contains("client secret", invalid[1].Detail, StringComparison.Ordinal); + Assert.Equal("file:/run/secrets/sql_password", entries[1].PlaintextPassword); + } + + [Fact] + public async Task AddServers_AWriteThatChangesNothingBecauseAnotherServerHoldsTheId_AnswersCollides() + { + var holderKey = StorageKeyOfHost(HolderHost); + Assert.NotEqual(holderKey, StorageKeyOfHost(CollidingHost)); + Assert.Equal( + ServerIdHelper.GetDeterministicHashCode(holderKey), ServerIdHelper.GetDeterministicHashCode(StorageKeyOfHost(CollidingHost))); + + var definitions = new FakeDefinitions(existing: new[] { holderKey }); + var result = await DarlingMcpServerAdminTools.AddServersAsync( + definitions, $"[{{\"host\":\"{CollidingHost}\"}},{{\"host\":\"other\"}}]", + (_, _) => Task.FromResult(HealthyProbeResult), CancellationToken.None); + + using var doc = JsonDocument.Parse(result); + var root = doc.RootElement; + Assert.Equal(2, root.GetProperty("requested").GetInt32()); + Assert.Equal(1, root.GetProperty("added").GetInt32()); + Assert.Equal(1, root.GetProperty("collided").GetInt32()); + Assert.Equal(0, root.GetProperty("failed").GetInt32()); + Assert.Equal(0, root.GetProperty("skipped").GetInt32()); + + var rows = root.GetProperty("results"); + Assert.Equal("collides", rows[0].GetProperty("status").GetString()); + Assert.Contains(HolderHost, rows[0].GetProperty("detail").GetString(), StringComparison.Ordinal); + Assert.Equal("added", rows[1].GetProperty("status").GetString()); + Assert.Equal(new[] { "other" }, definitions.Saved); + } + + [Fact] + public async Task AddServers_AWriteThatChangesNothingBecauseAConcurrentAddSavedTheSameServer_AnswersDuplicate() + { + /* The row is there when the write runs but was not there when the duplicate gate read the table. */ + var definitions = new FakeDefinitions(savedByAConcurrentWriter: new[] { StorageKeyOfHost("sql01") }); + var result = await DarlingMcpServerAdminTools.AddServersAsync( + definitions, "[{\"host\":\"sql01\"}]", (_, _) => Task.FromResult(HealthyProbeResult), CancellationToken.None); + + using var doc = JsonDocument.Parse(result); + var root = doc.RootElement; + Assert.Equal(0, root.GetProperty("added").GetInt32()); + Assert.Equal(1, root.GetProperty("skipped").GetInt32()); + Assert.Equal(0, root.GetProperty("collided").GetInt32()); + Assert.Equal("duplicate", root.GetProperty("results")[0].GetProperty("status").GetString()); + Assert.Empty(definitions.Saved); + } + + [Fact] + public async Task AddServers_AWriteThatChangesNothing_AndNoRowHoldsTheId_IsNotReportedAdded() + { + var definitions = new FakeDefinitions(swallowEveryInsert: true); + var result = await DarlingMcpServerAdminTools.AddServersAsync( + definitions, "[{\"host\":\"sql01\"}]", (_, _) => Task.FromResult(HealthyProbeResult), CancellationToken.None); + + using var doc = JsonDocument.Parse(result); + var root = doc.RootElement; + Assert.Equal(0, root.GetProperty("added").GetInt32()); + Assert.Equal(1, root.GetProperty("failed").GetInt32()); + Assert.Equal("not_saved", root.GetProperty("results")[0].GetProperty("status").GetString()); + } } /// @@ -838,6 +1106,72 @@ await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) = } } + /// + /// #4734 against the real table: two identities whose storage keys hash to ONE server_id (the hosts were + /// found by search; the test asserts they share the id). The second write hits + /// ON CONFLICT (server_id) DO NOTHING and changes 0 rows, so the tool has to read the holder back and + /// answer collides rather than added, and leave the first row alone. + /// + [Fact] + public async Task AddServers_ASecondIdentityWithTheSameServerId_AnswersCollides_AndLeavesTheFirstRowAlone_AgainstDevPostgres() + { + var cs = ConnectionString; + Assert.SkipWhen(string.IsNullOrEmpty(cs), + "Set DARLING_TEST_PG to a Postgres connection string (owner/superuser) to run the server-admin MCP tools live test."); + + var ct = TestContext.Current.CancellationToken; + using var connection = new NpgsqlConnection(cs); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + + var dataSourceConnectionString = new NpgsqlConnectionStringBuilder(cs) + { + SearchPath = "collect,config,public", + }.ConnectionString; + await using var postgres = NpgsqlDataSource.Create(dataSourceConnectionString); + + const string holderHost = "sql9jocsv"; + const string collidingHost = "sqlsvvqew"; + var sharedId = ServerIdHelper.GetDeterministicHashCode(ServerIdHelper.BuildStorageName(holderHost, null, false)); + Assert.Equal(sharedId, ServerIdHelper.GetDeterministicHashCode(ServerIdHelper.BuildStorageName(collidingHost, null, false))); + + await CleanupAsync(connection, ct, sharedId); + await DarlingMcpTestData.ExecAsync(connection, ct, "INSERT INTO config_service (id) VALUES (1) ON CONFLICT (id) DO NOTHING"); + + var bodySucceeded = false; + try + { + using (var doc = JsonDocument.Parse(await DarlingMcpServerAdminTools.AddServersAsync( + postgres, $"[{{\"host\":\"{holderHost}\"}}]", SuccessProbe, ct))) + { + Assert.Equal(1, doc.RootElement.GetProperty("added").GetInt32()); + } + + using (var doc = JsonDocument.Parse(await DarlingMcpServerAdminTools.AddServersAsync( + postgres, $"[{{\"host\":\"{collidingHost}\"}}]", SuccessProbe, ct))) + { + var root = doc.RootElement; + Assert.Equal(1, root.GetProperty("requested").GetInt32()); + Assert.Equal(0, root.GetProperty("added").GetInt32()); + Assert.Equal(1, root.GetProperty("collided").GetInt32()); + Assert.Equal(0, root.GetProperty("failed").GetInt32()); + var row = root.GetProperty("results")[0]; + Assert.Equal("collides", row.GetProperty("status").GetString()); + Assert.Contains(holderHost, row.GetProperty("detail").GetString(), StringComparison.Ordinal); + } + + Assert.Equal(1L, Convert.ToInt64(await ScalarAsync(connection, ct, $"SELECT count(*) FROM config_monitored_servers WHERE server_id = {sharedId}"))); + Assert.Equal(holderHost, await ScalarAsync(connection, ct, $"SELECT host FROM config_monitored_servers WHERE server_id = {sharedId}") as string); + + bodySucceeded = true; + } + finally + { + await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) => + await CleanupAsync(cleanup, cleanupCt, sharedId)); + } + } + private static async Task ScalarAsync(NpgsqlConnection connection, CancellationToken ct, string sql) { using var command = new NpgsqlCommand(sql, connection); diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs index 0df6f9a77d..ecf3d79f98 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs @@ -46,7 +46,12 @@ namespace PerformanceMonitor.Darling.Service.Mcp; /// serial to avoid a probe storm): for each server it validates the fields, skips an exact/case-variant DUPLICATE /// of an existing or earlier-in-batch server (status:"duplicate"), probes the connection (a failure is /// status:"connection_failed" and does NOT abort the batch), DPAPI-encrypts the SQL password or the service- -/// principal client secret, and INSERTs the row (status:"added"). Windows/integrated and managed-identity +/// principal client secret, and INSERTs the row (status:"added"). A save that throws is caught PER ENTRY: that +/// entry and every one after it are status:"not_saved" (the rest are not probed), and the entries before it +/// stay added in the results (#4734). An INSERT that changes no rows (its server_id is taken) is +/// answered from the row that holds the id — collides for a different identity, duplicate for the same +/// one — and never as added. A LITERAL password or client secret is invalid off Windows, where DPAPI is +/// not available (#4734). Windows/integrated and managed-identity /// auth store no secret; a service principal stores its client secret exactly like a SQL password; the INTERACTIVE /// Entra modes (MFA / device-code / default-credential) are rejected (status:"invalid") — they need a broker /// or a signed-in user and cannot run headless, whereas ServicePrincipal and ManagedIdentity are non-interactive @@ -110,18 +115,23 @@ private static Task DefaultProbeAsync(MonitoredServer ser "read_only_intent (bool, default false); multi_subnet_failover (bool, default false). Servers are processed " + "IN ORDER, one at a time. A case-variant or exact duplicate of an already-monitored server (or an earlier " + "entry in the same array) is skipped as status \"duplicate\". A server that fails to connect is recorded as " + - "status \"connection_failed\" and does NOT stop the rest of the batch. The INTERACTIVE Microsoft Entra modes " + + "status \"connection_failed\" and does NOT stop the rest of the batch. If saving an entry fails, that entry " + + "and every entry after it are reported as \"not_saved\" without being tested, and the entries before it stay " + + "added. The INTERACTIVE Microsoft Entra modes " + "(MFA / device-code / default-credential) are rejected (status \"invalid\") — they need a broker or a " + "signed-in user and cannot run headless; ServicePrincipal and ManagedIdentity are non-interactive and are " + "supported. A server whose probed connection lands in a database that ANOTHER monitored server already " + "covers (it names one database but connects to a different one, e.g. a wrong Initial Catalog) is refused as " + "status \"collides\" — adding it would store one database's history under two identities and alert twice. " + + "So is a server whose id is already held by a different server (two identities that hash to one id). " + "A SQL password or service-principal client secret is encrypted at rest (DPAPI, the service identity) and " + - "is never returned. Returns {requested:N, added:N, skipped:N, collided:N, failed:N, results:[{server, " + - "status:\"added\"|\"duplicate\"|\"collides\"|\"connection_failed\"|\"invalid\", detail}]}. requested is " + + "is never returned. Where DPAPI is not available (Linux) a literal password or client secret is rejected as " + + "\"invalid\": give an env:NAME or file:/run/secrets/ reference instead. " + + "Returns {requested:N, added:N, skipped:N, collided:N, failed:N, results:[{server, " + + "status:\"added\"|\"duplicate\"|\"collides\"|\"connection_failed\"|\"not_saved\"|\"invalid\", detail}]}. requested is " + "the number of entries you sent and the four counters SUM to it — every entry lands in exactly one: " + - "\"added\" → added, \"duplicate\" → skipped, \"collides\" → collided, \"connection_failed\" and " + - "\"invalid\" → failed. Only added servers are monitored; read the other three counters before treating " + + "\"added\" → added, \"duplicate\" → skipped, \"collides\" → collided, \"connection_failed\", " + + "\"not_saved\" and \"invalid\" → failed. Only added servers are monitored; read the other three counters before treating " + "the batch as done. An added server's detail reports what the probe found — for a PostgreSQL target that " + "includes writer-vs-reader, Aurora-vs-not, and how many of the PostgreSQL collectors apply to it. NOTE: the " + "password travels to this endpoint in the request; on a LAN use the documented TLS reverse proxy. " + @@ -138,8 +148,14 @@ public static Task AddServers( /// seam) + encrypts + INSERTs, aggregating a per-server result. Structural validation /// runs BEFORE any store access, and when NO structurally-valid candidate remains the store is never opened — /// so a call whose entries are all invalid (bad field, MFA auth) returns without a connection or a probe. + internal static Task AddServersAsync( + NpgsqlDataSource postgres, string servers_json, ServerProbe probe, CancellationToken cancellationToken) => + AddServersAsync(new PostgresServerDefinitions(postgres), servers_json, probe, cancellationToken); + + /// The same flow over an injected , so a test can stand in a + /// definitions table that faults on a chosen write or swallows one without a live database. internal static async Task AddServersAsync( - NpgsqlDataSource postgres, string servers_json, ServerProbe probe, CancellationToken cancellationToken) + IServerDefinitions definitions, string servers_json, ServerProbe probe, CancellationToken cancellationToken) { try { @@ -161,7 +177,7 @@ reports only the per-entry invalids. */ /* Seed the case-folded dedupe gate from the authoritative store rows FIRST, then partition the batch — a duplicate (of an existing server OR an earlier entry in this batch, first occurrence wins) is skipped WITHOUT a probe, exactly as the bulk dialog (#1549) does. */ - var existingKeys = await LoadExistingStorageKeysAsync(postgres, cancellationToken); + var existingKeys = await definitions.LoadStorageKeysAsync(cancellationToken); var (ready, duplicates) = PartitionDuplicates(entries, existingKeys); results.AddRange(duplicates); @@ -175,8 +191,21 @@ a duplicate (of an existing server OR an earlier entry in this batch, first occu claimed.Add(entry.StorageKey); } + /* #4734: set when a save throws. Every entry after that is answered WITHOUT a probe: the table is not + taking writes, and a probe waits out a connect timeout for a server that could not be saved anyway. + The entries already saved stay in the results as added — they are in the table and being collected. */ + var saveFailed = false; + foreach (var entry in ready) { + if (saveFailed) + { + results.Add(new ServerResult(entry.Order, entry.DisplayName, AddStatus.NotSaved, + "Not saved and not tested: an earlier entry in this batch could not be saved, so the rest of " + + "the batch was not attempted. Send this entry again.")); + continue; + } + /* Validate the connection IN-PROCESS (the service holds the network path + credentials). A failure is recorded and the batch CONTINUES — one unreachable server never aborts the rest. */ var probeResult = await probe(entry.ProbeConfig, cancellationToken); @@ -208,9 +237,30 @@ reported at every connect by #2277's tripwire. /* DPAPI-encrypt the SQL password for storage (the service identity encrypts here and decrypts it during collection, so it round-trips); Windows-auth servers store no secret. The plaintext never leaves this method — it is not logged, not echoed in a result. */ - var encryptedPassword = ProtectPasswordForStorage(entry.PlaintextPassword); - await InsertServerAsync(postgres, entry, encryptedPassword, cancellationToken); - results.Add(new ServerResult(entry.Order, entry.DisplayName, AddStatus.Added, DescribeProbe(probeResult))); + try + { + var encryptedPassword = ProtectPasswordForStorage(entry.PlaintextPassword); + var rowsWritten = await definitions.InsertAsync(entry, encryptedPassword, cancellationToken); + if (rowsWritten > 0) + { + results.Add(new ServerResult(entry.Order, entry.DisplayName, AddStatus.Added, DescribeProbe(probeResult))); + } + else + { + /* #4734: ON CONFLICT (server_id) DO NOTHING wrote nothing, so this entry is NOT in the table. + Ask who holds its id rather than report "added" for a server nothing will collect. */ + var holder = await definitions.ReadStorageKeyAsync(ServerIdOf(entry), cancellationToken); + results.Add(ClassifyUnwrittenInsert(entry, holder)); + } + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + /* #4734: catch PER ENTRY. The outer catch below used to be the only one, so a fault on entry N + dropped the list that recorded entries 1 to N-1 as added and answered one error for a call + that had saved them — and the retry then reported them as duplicates. */ + saveFailed = true; + results.Add(new ServerResult(entry.Order, entry.DisplayName, AddStatus.NotSaved, $"Not saved: {ex.Message}")); + } } return Aggregate(results); @@ -393,10 +443,14 @@ internal static class AddStatus /// without a probe (#1549). public const string Duplicate = "duplicate"; - /// Probed, but its connection reached a database another registration already covers (#2280); NOT - /// added. + /// Probed, but its connection reached a database another registration already covers (#2280), or its + /// server_id is already held by a different registration (#4734); NOT added. public const string Collides = "collides"; + /// The save did not happen (#4734): the write threw, wrote nothing with no row to explain it, or an + /// earlier entry's save threw and this one was never attempted. NOT added. + public const string NotSaved = "not_saved"; + /// The in-process probe could not connect; NOT added, the batch continued. public const string ConnectionFailed = "connection_failed"; @@ -427,6 +481,7 @@ internal static class AddStatus [AddStatus.Duplicate] = "skipped", [AddStatus.Collides] = "collided", [AddStatus.ConnectionFailed] = "failed", + [AddStatus.NotSaved] = "failed", [AddStatus.Invalid] = "failed", }; @@ -540,7 +595,13 @@ internal static RemovalTarget ResolveForRemoval(IReadOnlyListWholeError when the whole payload is unusable (not JSON, not an array, or empty), for which the caller /// returns a single {status:"invalid"} without opening the store. /// - internal static (List Entries, List Invalid, string? WholeError) ParseRequest(string servers_json) + internal static (List Entries, List Invalid, string? WholeError) ParseRequest(string servers_json) => + ParseRequest(servers_json, OperatingSystem.IsWindows()); + + /// with the platform named, so a test can ask what either platform + /// answers for a literal password without running on it. + internal static (List Entries, List Invalid, string? WholeError) ParseRequest( + string servers_json, bool isWindows) { var entries = new List(); var invalid = new List(); @@ -567,7 +628,7 @@ internal static (List Entries, List Invalid, st for (var i = 0; i < array.Count; i++) { - var (entry, result) = ParseEntry(i, array[i]); + var (entry, result) = ParseEntry(i, array[i], isWindows); if (entry != null) { entries.Add(entry); @@ -585,7 +646,7 @@ internal static (List Entries, List Invalid, st /// problem. The service honors Windows, SQL, and the two non-interactive Entra modes (ServicePrincipal, /// ManagedIdentity); the interactive Entra modes (MFA/device-code/default-credential) are rejected — they /// cannot run headless (#3484). - private static (ParsedServerEntry? Entry, ServerResult? Result) ParseEntry(int index, JsonNode? node) + private static (ParsedServerEntry? Entry, ServerResult? Result) ParseEntry(int index, JsonNode? node, bool isWindows) { if (node is not JsonObject obj) { @@ -663,6 +724,14 @@ the client secret as the password (DPAPI-encrypted after a successful probe, exa ? "password is required for ServicePrincipal authentication (the client secret)." : "password is required for SQL authentication.")); } + + /* #4734: refuse a literal where it cannot be encrypted, HERE, before the probe and the write. Left to + ProtectPasswordForStorage it threw after the probe, in the middle of the batch. */ + var refusal = LiteralSecretRefusal(plaintextPassword, isWindows, isSp); + if (refusal != null) + { + return (null, Invalid(refusal)); + } } else if (storeAuth == ServerStoreAuth.ManagedIdentity) { @@ -798,6 +867,44 @@ entry does not collide with a SQL Server one on the same host. */ "registration instead."; } + /// + /// PURE answer for a write that changed no rows (#4734). server_id is a 32-bit hash of the storage key and + /// the insert is ON CONFLICT (server_id) DO NOTHING, so a write can save nothing without throwing: two + /// different keys can hash to one id, and a concurrent add of the same server can land between the duplicate + /// gate's read and this write. is the storage key of the row that holds the id + /// now, or null when none does. + /// + /// + /// A DIFFERENT key holds the id: collides. The entry is not a duplicate — its identity is not + /// registered — but it cannot be saved under this identity. + /// The SAME key holds it (compared case-folded, as the duplicate gate is): duplicate, the server is + /// registered, by a concurrent add. + /// No row holds it (removed between the write and the read): not_saved, because the write saved + /// nothing and nothing explains why. Never "added". + /// + /// + internal static ServerResult ClassifyUnwrittenInsert(ParsedServerEntry entry, string? holderStorageKey) + { + if (holderStorageKey is null) + { + return new ServerResult(entry.Order, entry.DisplayName, AddStatus.NotSaved, + "Not saved: the write changed no rows, and no server holds this entry's id now (it was removed in " + + "between). Send this entry again."); + } + + if (string.Equals(holderStorageKey, entry.StorageKey, StringComparison.OrdinalIgnoreCase)) + { + return new ServerResult(entry.Order, entry.DisplayName, AddStatus.Duplicate, + "Already monitored: another add of this server saved it while this call ran; skipped."); + } + + return new ServerResult(entry.Order, entry.DisplayName, AddStatus.Collides, + $"Not added: this server's id (a 32-bit hash of its identity) is already held by a different monitored " + + $"server, '{holderStorageKey}'. The two identities differ but hash to the same id, so both cannot be " + + "saved. Register this one under a differently spelled address (for example its fully qualified name) so " + + "its identity, and its id, differ."); + } + /// /// PURE dedupe partition — the case-folded gate seeded with the /// existing store keys, first-occurrence-wins within the batch (the #1549 idiom). Returns the Ready @@ -844,7 +951,7 @@ internal static (List Ready, List Duplicates) P /// alert_delivery_mode_override default to NULL (inherit the globals), monthly_cost_usd/ /// excluded_databases are the neutral defaults, and is_enabled is TRUE (collection starts at /// once). ON CONFLICT DO NOTHING guards a race with a concurrent writer — the dedupe gate is the primary - /// guard. + /// guard. DO NOTHING is silent, so the caller checks the rows changed (#4734): 0 means nothing was saved. public const string InsertServerSql = @" INSERT INTO config_monitored_servers ( server_id, name, host, database, auth, username, encrypted_password, encrypt_mode, @@ -861,18 +968,41 @@ private static async Task> LoadExistingStorageKeysAsync(NpgsqlDataS await using var reader = await command.ExecuteReaderAsync(cancellationToken); while (await reader.ReadAsync(cancellationToken)) { - var host = reader.GetString(0); - var database = reader.IsDBNull(1) ? null : reader.GetString(1); - var readOnlyIntent = !reader.IsDBNull(2) && reader.GetBoolean(2); - var engine = reader.IsDBNull(3) ? null : reader.GetString(3); - var port = reader.IsDBNull(4) ? 0 : reader.GetInt32(4); - keys.Add(ServerIdHelper.BuildStorageName(host, database, readOnlyIntent, engine, port)); + keys.Add(StorageKeyOf(reader)); } return keys; } - private static async Task InsertServerAsync( + /// The storage key of the row a reader is positioned on, from the five identity columns + /// and both read, in that order — one mapping for + /// both so the dedupe gate and the lookup by id cannot disagree about what a row's identity is. + private static string StorageKeyOf(NpgsqlDataReader reader) + { + var host = reader.GetString(0); + var database = reader.IsDBNull(1) ? null : reader.GetString(1); + var readOnlyIntent = !reader.IsDBNull(2) && reader.GetBoolean(2); + var engine = reader.IsDBNull(3) ? null : reader.GetString(3); + var port = reader.IsDBNull(4) ? 0 : reader.GetInt32(4); + return ServerIdHelper.BuildStorageName(host, database, readOnlyIntent, engine, port); + } + + /// The identity columns of the one row holding a given server_id. + internal const string ServerByIdSql = + "SELECT host, database, read_only_intent, engine, port FROM config_monitored_servers WHERE server_id = $1"; + + /// The storage key of the row that holds , or null when no row does. + private static async Task ReadStorageKeyByIdAsync( + NpgsqlDataSource postgres, int serverId, CancellationToken cancellationToken) + { + await using var command = postgres.CreateCommand(ServerByIdSql); + command.CommandTimeout = McpCommandDeadlines.ReadSeconds; + command.Parameters.Add(new NpgsqlParameter { TypedValue = serverId }); // $1 + await using var reader = await command.ExecuteReaderAsync(cancellationToken); + return await reader.ReadAsync(cancellationToken) ? StorageKeyOf(reader) : null; + } + + private static async Task InsertServerAsync( NpgsqlDataSource postgres, ParsedServerEntry entry, string? encryptedPassword, CancellationToken cancellationToken) { var config = entry.ProbeConfig; @@ -880,7 +1010,7 @@ private static async Task InsertServerAsync( await using var command = postgres.CreateCommand(InsertServerSql); command.CommandTimeout = McpCommandDeadlines.ReadSeconds; - command.Parameters.Add(new NpgsqlParameter { TypedValue = ServerIdHelper.GetDeterministicHashCode(entry.StorageKey) }); // $1 + command.Parameters.Add(new NpgsqlParameter { TypedValue = ServerIdOf(entry) }); // $1 command.Parameters.Add(new NpgsqlParameter { TypedValue = config.Name }); // $2 command.Parameters.Add(new NpgsqlParameter { TypedValue = config.Host }); // $3 AddNullableText(command, config.Database); // $4 @@ -896,18 +1026,80 @@ private static async Task InsertServerAsync( command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Timestamp, Value = now }); // $14 command.Parameters.Add(new NpgsqlParameter { TypedValue = config.Engine }); // $15 command.Parameters.Add(new NpgsqlParameter { TypedValue = config.Port }); // $16 - await command.ExecuteNonQueryAsync(cancellationToken); + return await command.ExecuteNonQueryAsync(cancellationToken); + } + + /// The server_id an entry is stored under: the deterministic hash of its storage key, the same + /// id the collectors derive, so a tool-added row joins the collected data. + private static int ServerIdOf(ParsedServerEntry entry) => ServerIdHelper.GetDeterministicHashCode(entry.StorageKey); + + /// + /// The storage operations add_servers performs, as a seam: the real one is + /// , and a test stands in one that faults on a chosen write or swallows a + /// write, neither of which can be produced on demand against a live database. + /// + internal interface IServerDefinitions + { + /// The storage key of every existing server, to seed the dedupe gate. + Task> LoadStorageKeysAsync(CancellationToken cancellationToken); + + /// Writes one entry and returns how many rows the write changed: 1 when it was saved, 0 when + /// ON CONFLICT (server_id) DO NOTHING found the id already taken and wrote nothing. + Task InsertAsync(ParsedServerEntry entry, string? encryptedPassword, CancellationToken cancellationToken); + + /// The storage key of the row that holds , or null when none does. + Task ReadStorageKeyAsync(int serverId, CancellationToken cancellationToken); + } + + /// The definitions table itself: config_monitored_servers. + private sealed class PostgresServerDefinitions : IServerDefinitions + { + private readonly NpgsqlDataSource _postgres; + + public PostgresServerDefinitions(NpgsqlDataSource postgres) => _postgres = postgres; + + public Task> LoadStorageKeysAsync(CancellationToken cancellationToken) => + LoadExistingStorageKeysAsync(_postgres, cancellationToken); + + public Task InsertAsync(ParsedServerEntry entry, string? encryptedPassword, CancellationToken cancellationToken) => + InsertServerAsync(_postgres, entry, encryptedPassword, cancellationToken); + + public Task ReadStorageKeyAsync(int serverId, CancellationToken cancellationToken) => + ReadStorageKeyByIdAsync(_postgres, serverId, cancellationToken); } /* ─────────────────────────────── helpers ─────────────────────────────── */ + /// + /// PURE platform check for a SQL password or service-principal client secret (#4734): null when the value can be + /// stored on this platform, otherwise the refusal sentence. A LITERAL is encrypted with DPAPI, which is Windows- + /// only, so off Windows it cannot be saved; an env:/file: reference (#1804) is stored as given and + /// needs no encryption, so it is accepted everywhere. asks this before any probe or + /// write, which makes the entry invalid up front instead of throwing from + /// after the probe, in the middle of a batch. + /// + internal static string? LiteralSecretRefusal(string? secret, bool isWindows, bool isServicePrincipal) + { + if (isWindows || string.IsNullOrEmpty(secret) || DarlingSecretSource.IsReference(secret)) + { + return null; + } + + var noun = isServicePrincipal ? "client secret" : "password"; + return $"A literal {noun} cannot be saved on this platform: encrypting it needs Windows DPAPI. Give the " + + $"{noun} as an env:NAME or file:/run/secrets/ reference instead (for example " + + "file:/run/secrets/sql_password)."; + } + /// Prepares a SQL password for storage: an env:/file: secret REFERENCE (#1804) is /// stored VERBATIM — a reference is a pointer, not a secret; the secret stays in the mounted file or /// environment variable, which is the whole Linux/compose contract and needs no DPAPI (#2087: before this, /// add_servers refused ALL SQL-auth passwords off-Windows, dead-ending the designed onboarding path /// for compose deployments — the store-authoritative control plane means darling.json edits do not add /// servers after first seed). A LITERAL password is DPAPI-encrypted and therefore Windows-only, with the - /// refusal now pointing at references as the cross-platform alternative. Null for Windows auth (no secret). + /// refusal now pointing at references as the cross-platform alternative. refuses a literal + /// off Windows first (, #4734), so a request never reaches this throw; it stays as + /// the backstop for any other caller. Null for Windows auth (no secret). /// The plaintext is not logged and never leaves this method. internal static string? ProtectPasswordForStorage(string? plaintextPassword) { diff --git a/Darling/README.md b/Darling/README.md index 4e2998de51..7c71ff456f 100644 --- a/Darling/README.md +++ b/Darling/README.md @@ -651,7 +651,7 @@ The embedded MCP server, over Streamable HTTP bound to `localhost` by default (s - **Alert-tuning write tools (Darling-only)** — `update_alert_settings`, `create_mute_rule`, `update_mute_rule`, `delete_mute_rule`, `set_mute_rule_enabled`, and the two route writes `set_notification_route_enabled` / `delete_notification_route` (a route leaves force or is removed; authoring one — a destination URL — is the Viewer's Settings window only, because a destination is a bearer secret the `mcp` role never holds) let an MCP client TUNE the alert engine the fleet shares — the SAME config `get_alert_settings` / `get_mute_rules` read and the Viewer's Settings window writes. `update_alert_settings` is a PARTIAL update of the alert configuration: read via `get_alert_settings`, change fields, and send only those back in the same nested shape; every field is validated against the SAME ranges/enums the Settings window enforces BEFORE any write, an out-of-range or unknown field returns `{status:"invalid"}` and writes nothing, and the write self-bumps `config_version` so the running service hot-reloads within one collection sweep. `create_mute_rule` / `update_mute_rule` / `delete_mute_rule` / `set_mute_rule_enabled` reuse the SAME `PgMuteRuleStore` `get_mute_rules` reads through (and the same GUID id-generation the Viewer's mute-create path uses). `set_mute_rule_enabled` is the headless twin of the Viewer's mute-rule checkbox: it writes `enabled` and nothing else, so a rule can leave force and return keeping its id, its reason and the `created_at_utc` the `Stale Mute Rules` self-alert ages it from — where a delete-and-re-create resets that clock and delivers the suppressed alerts in between. `update_mute_rule` is the headless twin of the Viewer's Edit dialog on the same terms: a PARTIAL update (send only the fields to change, in the shape `get_mute_rules` returns; an explicit JSON `null` clears a field, so a bounded rule can be made permanent again) written through the store's own full-row `UpdateAsync` — whose SET list does not name `created_at_utc`, so no edit, however sweeping, resets the staleness clock the way the delete/recreate workaround did for every field the enable flag cannot express. `update_alert_settings` spans the two singleton config rows the alert engine reads: every knob is on `config_alert_settings` except the per-fingerprint DELIVERY cooldown, which is stored as `config_notification.email_cooldown_minutes` and reported and accepted as `delivery.cooldown_minutes` — a channel-neutral name, because that one number is the only throttle on a Slack / Teams / PagerDuty / generic-webhook post as well as on email, so a headless deployment with no SMTP configured at all is still governed by it (the stored spelling is accepted as a write-only alias). Note the two cooldowns are different stages: top-level `cooldown_minutes` gates whether the engine FIRES, `delivery.cooldown_minutes` bounds the resulting post — twice over, once per alert FINGERPRINT and once per METRIC across the whole fleet for a RE-notification, so one fault on forty servers does not cost forty posts an hour and the servers that second bound holds back are named on the post that does go out. A first notice is never held back by either bound, and `delivery.mode` `PerEvent` opts out of the per-metric one. None touches a monitored SQL Server or the collected data — only the shared alert configuration; SMTP/webhook delivery credentials are out of scope (the `mcp` role can read and write exactly the one non-secret cooldown column of the notification table, and cannot read or write any secret column). It is part of what the MCP token gates — see [What a token can reach](#opt-in-network-endpoints-lan) below. -- **Server-onboarding write tools (Darling-only)** — `add_servers` (BULK) and `remove_server` let an MCP client stand up or tear down FLEET monitoring conversationally ("monitor these twenty servers with this login"), the service-side twin of the Viewer's Add / Manage Servers dialogs. `add_servers` takes a JSON **array** of server objects (`host` required; optional `display_name` / `database` / `read_only_intent` / `multi_subnet_failover`; `auth` `Windows`/`SQL` with `username`+`password` for SQL; and the exposed TLS options `encrypt_mode` `Optional`/`Mandatory`/`Strict` + `trust_server_certificate`) and processes them **in order**: it validates each entry, PROBES the connection in-process (reusing the same `DarlingServerConnector.ProbeAsync` the `--test-connection` verb runs — the service holds the network path + credentials, so no `test_connect` command plane is needed), skips a case-folded duplicate (`duplicate`) of an already-monitored server or an earlier entry, DPAPI-encrypts the SQL password (the service identity, so it round-trips at collection time), and INSERTs the row mirroring the service's own seed shape. A server that fails to connect is `connection_failed` and the batch continues; Entra/MFA/Service-Principal/Managed-Identity auth is `invalid` (the service connects with Windows or SQL only). `remove_server` DELETEs a monitored server by name (resolved the same way every `server_name` is) — already-collected history is kept. Both write only the monitoring store's `config.config_monitored_servers` registry; neither runs anything on a monitored server beyond the one-time probe. **The SQL password travels to the endpoint inside `add_servers`' request** and is DPAPI-encrypted at rest (never returned) — it is part of what the MCP token gates, and it puts a credential on the wire; see [What a token can reach](#opt-in-network-endpoints-lan) below. +- **Server-onboarding write tools (Darling-only)** — `add_servers` (BULK) and `remove_server` let an MCP client stand up or tear down FLEET monitoring conversationally ("monitor these twenty servers with this login"), the service-side twin of the Viewer's Add / Manage Servers dialogs. `add_servers` takes a JSON **array** of server objects (`host` required; optional `display_name` / `database` / `read_only_intent` / `multi_subnet_failover`; `auth` `Windows`/`SQL` with `username`+`password` for SQL; and the exposed TLS options `encrypt_mode` `Optional`/`Mandatory`/`Strict` + `trust_server_certificate`) and processes them **in order**: it validates each entry, PROBES the connection in-process (reusing the same `DarlingServerConnector.ProbeAsync` the `--test-connection` verb runs — the service holds the network path + credentials, so no `test_connect` command plane is needed), skips a case-folded duplicate (`duplicate`) of an already-monitored server or an earlier entry, DPAPI-encrypts the SQL password (the service identity, so it round-trips at collection time), and INSERTs the row mirroring the service's own seed shape. A server that fails to connect is `connection_failed` and the batch continues. `auth` also takes the two non-interactive Microsoft Entra modes (#3484). With `ServicePrincipal`, `username` is the app's client id and `password` is its client secret. The other mode is `ManagedIdentity`. Only the interactive Entra modes (MFA, device code, default credential) are `invalid`, because they need a signed-in user and cannot run headless. Off Windows, a literal SQL password or client secret is refused as `invalid` too, because encryption at rest is Windows DPAPI. Give an `env:NAME` or `file:/run/secrets/` reference instead, the same two forms that `darling.json` takes for every secret. A server is refused as `collides` when its probed connection lands in a database that another monitored server already covers. That happens when it names one database but connects to a different one, such as a wrong Initial Catalog. Adding it gives one database two identities, so its history is stored twice and its alerts fire twice. A server whose id is already held by a different server is refused as `collides` as well, because two identities can hash to one id. Nothing is written for a `collides` entry. If saving an entry fails, that entry and every entry after it are `not_saved` and are not probed. The entries before it stay `added`. `remove_server` DELETEs a monitored server by name (resolved the same way every `server_name` is) — already-collected history is kept. Both write only the monitoring store's `config.config_monitored_servers` registry; neither runs anything on a monitored server beyond the one-time probe. **The SQL password travels to the endpoint inside `add_servers`' request** and is DPAPI-encrypted at rest (never returned) — it is part of what the MCP token gates, and it puts a credential on the wire; see [What a token can reach](#opt-in-network-endpoints-lan) below. | Key | Default | Notes | |---|---|---|