Review 1.5: synthetic check runner: network surprises are results, AX process never outlives the agent

- An HTTP answer that breaks off while it is read (HttpIOException/IOException) is a
  failed check ("Svaret blev afbrudt, mens det blev læst: …"), not an exception that
  made the check rerun every tick. Other unforeseen network errors (IOException,
  SocketException, HttpRequestException, AuthenticationException) become
  "Forbindelsen fejlede: …"; one that hits the check's deadline is "Intet svar inden
  for N sekunder". DNS: an invalid name (ArgumentException) is a failed lookup.
- The HTTP "Adresse" detail (a URL may carry query secrets) and the AX "Object server"
  detail are Sensitive, so the API redacts them.
- SyntheticCheckRun's compiler-generated ToString no longer prints the SQL password.
- ProcessRunner (AX Business Connector logon): the PowerShell process tree is killed in
  a finally whenever it has not exited, also when the agent stops (before: only on the
  check's timeout, leaving an orphaned process and AX session). Output and errors are
  capped at 64 K characters each; the rest is read and dropped so the process never
  blocks on a full pipe.
- Tests: broken body, password redaction, bounded output, killed on agent stop.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

src/SQLInsight.Servers/Synthetic/AxBusinessConnector.cs

@@ -64,9 +64,14 @@ public interface IProcessRunner
         CancellationToken cancellationToken);
 }
 
-/// <summary>Runs a process and kills it (with its children) when it takes too long.</summary>
+/// <summary>
+/// Runs a process and kills it (with its children) when it takes too long or the agent stops; never leaves one behind.
+/// At most <see cref="MaxOutputChars"/> of its output and of its errors are kept.
+/// </summary>
 public sealed class ProcessRunner : IProcessRunner
 {
+    public const int MaxOutputChars = 64 * 1024;
+
     public async Task<ProcessResult> RunAsync(string fileName, IReadOnlyList<string> arguments, IReadOnlyDictionary<string, string> environment, TimeSpan timeout,
         CancellationToken cancellationToken)
     {
@@ -92,21 +97,65 @@ public sealed class ProcessRunner : IProcessRunner
         }
 
         using var process = Process.Start(start) ?? throw new InvalidOperationException($"{fileName} kunne ikke startes.");
-        var output = process.StandardOutput.ReadToEndAsync(cancellationToken);
+        var output = ReadBoundedAsync(process.StandardOutput, MaxOutputChars);
-        var error = process.StandardError.ReadToEndAsync(cancellationToken);
+        var error = ReadBoundedAsync(process.StandardError, MaxOutputChars);
         using var deadline = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
         deadline.CancelAfter(timeout);
         try
         {
             await process.WaitForExitAsync(deadline.Token).ConfigureAwait(false);
+            return new ProcessResult(process.ExitCode, await output.WaitAsync(deadline.Token).ConfigureAwait(false), await error.WaitAsync(deadline.Token).ConfigureAwait(false),
+                TimedOut: false);
         }
         catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested)
         {
-            process.Kill(entireProcessTree: true);
             return new ProcessResult(-1, string.Empty, string.Empty, TimedOut: true);
         }
+        finally
+        {
+            // Timed out, or the agent stops: a Business Connector process left running would keep its AX session.
+            Kill(process);
+            Observe(output);
+            Observe(error);
+        }
+    }
 
-        return new ProcessResult(process.ExitCode, await output.ConfigureAwait(false), await error.ConfigureAwait(false), TimedOut: false);
+    private static void Kill(Process process)
+    {
+        try
+        {
+            if (!process.HasExited)
+            {
+                process.Kill(entireProcessTree: true);
+            }
+        }
+        catch (Exception ex) when (ex is InvalidOperationException or System.ComponentModel.Win32Exception or NotSupportedException)
+        {
+            // It ended on its own in the meantime, or cannot be killed: nothing more to do.
+        }
+    }
+
+    /// <summary>A reader no longer waited for ends when the process does; its error (the stream was closed) is not left unobserved.</summary>
+    private static void Observe(Task task) =>
+        _ = task.ContinueWith(static t => _ = t.Exception, CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously,
+            TaskScheduler.Default);
+
+    /// <summary>Reads to the end, keeping at most <paramref name="maxChars"/>: the rest is read and dropped, so the process never blocks on a full pipe.</summary>
+    internal static async Task<string> ReadBoundedAsync(TextReader reader, int maxChars)
+    {
+        var builder = new StringBuilder();
+        var buffer = new char[4096];
+        int read;
+        while ((read = await reader.ReadAsync(buffer.AsMemory()).ConfigureAwait(false)) > 0)
+        {
+            var keep = Math.Min(read, maxChars - builder.Length);
+            if (keep > 0)
+            {
+                builder.Append(buffer, 0, keep);
+            }
+        }
+
+        return builder.ToString();
     }
 }
 

src/SQLInsight.Servers/Synthetic/SyntheticCheckRunner.cs

@@ -14,7 +14,15 @@ using SQLInsight.Core.SyntheticChecks;
 namespace SQLInsight.Servers.Synthetic;
 
 /// <summary>One run of a check: its settings, the opened SQL login password (if any) and how long it may take.</summary>
-public sealed record SyntheticCheckRun(string Kind, SyntheticCheckSettings Settings, TimeSpan Timeout, string? Password = null);
+public sealed record SyntheticCheckRun(string Kind, SyntheticCheckSettings Settings, TimeSpan Timeout, string? Password = null)
+{
+    /// <summary>The password never ends up in a log or an exception through ToString.</summary>
+    private bool PrintMembers(StringBuilder builder)
+    {
+        builder.Append(CultureInfo.InvariantCulture, $"Kind = {Kind}, Settings = {Settings}, Timeout = {Timeout}, Password = {(Password is null ? "null" : "***")}");
+        return true;
+    }
+}
 
 /// <summary>
 /// Runs synthetic checks the way a user or an application would: connect to a port, fetch a page, log on to SQL Server
@@ -60,10 +68,16 @@ public sealed class SyntheticCheckRunner(ISqlConnectionFactory sqlConnections, I
                 _ => Failed(at, 0, $"Ukendt type tjek: {run.Kind}."),
             };
         }
-        catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested)
+        catch (Exception ex) when (!cancellationToken.IsCancellationRequested
+                                   && (ex is OperationCanceledException || (deadline.IsCancellationRequested && ex is IOException or SocketException or HttpRequestException)))
         {
             return Failed(at, Elapsed(started), $"Intet svar inden for {run.Timeout.TotalSeconds.ToString("0", CultureInfo.InvariantCulture)} sekunder.");
         }
+        catch (Exception ex) when (ex is IOException or SocketException or HttpRequestException or AuthenticationException)
+        {
+            // The network failed in a way the check's own handling did not foresee: still the check's result, not an error.
+            return Failed(at, Elapsed(started), "Forbindelsen fejlede: " + ex.Message);
+        }
     }
 
     private int Elapsed(long started) => (int)Math.Min(int.MaxValue, time.GetElapsedTime(started).TotalMilliseconds);
@@ -146,7 +160,7 @@ public sealed class SyntheticCheckRunner(ISqlConnectionFactory sqlConnections, I
         using var client = new HttpClient(handler) { Timeout = Timeout.InfiniteTimeSpan };
         using var request = new HttpRequestMessage(HttpMethod.Get, url);
         request.Headers.UserAgent.ParseAdd(UserAgent);
-        var details = new List<SyntheticCheckDetail> { new("Adresse", url.ToString()) };
+        var details = new List<SyntheticCheckDetail> { new("Adresse", url.ToString(), Sensitive: true) };
         try
         {
             using var response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);
@@ -199,6 +213,21 @@ public sealed class SyntheticCheckRunner(ISqlConnectionFactory sqlConnections, I
                 CertificateSubject = certificate?.Subject,
             };
         }
+        catch (IOException ex) when (!cancellationToken.IsCancellationRequested)
+        {
+            // The connection broke while the answer was read (HttpIOException and the like).
+            AddCertificate(details, certificate, certificateErrors);
+            return new SyntheticCheckResult
+            {
+                At = at,
+                Success = false,
+                DurationMs = Elapsed(started),
+                Message = $"Svaret blev afbrudt, mens det blev læst: {ex.Message}",
+                Details = details,
+                CertificateExpiresAt = certificate is null ? null : new DateTimeOffset(certificate.NotAfter.ToUniversalTime(), TimeSpan.Zero),
+                CertificateSubject = certificate?.Subject,
+            };
+        }
         finally
         {
             certificate?.Dispose();
@@ -395,7 +424,7 @@ public sealed class SyntheticCheckRunner(ISqlConnectionFactory sqlConnections, I
         {
             addresses = await Dns.GetHostAddressesAsync(host, cancellationToken).ConfigureAwait(false);
         }
-        catch (SocketException ex)
+        catch (Exception ex) when (ex is SocketException or ArgumentException)
         {
             return Failed(at, Elapsed(started), $"Navnet {host} kunne ikke slås op: {ex.Message}");
         }
@@ -455,7 +484,7 @@ public sealed class SyntheticCheckRunner(ISqlConnectionFactory sqlConnections, I
         }
 
         details.Add(new SyntheticCheckDetail("Business Connector", $"{installation.Version} ({installation.Path})"));
-        details.Add(new SyntheticCheckDetail("Object server", objectServer));
+        details.Add(new SyntheticCheckDetail("Object server", objectServer, Sensitive: true));
         if (!string.IsNullOrWhiteSpace(settings.Company))
         {
             details.Add(new SyntheticCheckDetail("Firma", settings.Company));

tests/SQLInsight.Servers.Tests/ProcessRunnerTests.cs

@@ -0,0 +1,77 @@
+using System.Diagnostics;
+using SQLInsight.Servers.Synthetic;
+
+namespace SQLInsight.Servers.Tests;
+
+/// <summary>The process the AX logon runs in: its output is bounded, and it never outlives the agent.</summary>
+public class ProcessRunnerTests
+{
+    [Fact]
+    public async Task Process_output_is_capped_but_read_to_the_end()
+    {
+        using var reader = new StringReader(new string('x', ProcessRunner.MaxOutputChars * 3) + "end");
+
+        var text = await ProcessRunner.ReadBoundedAsync(reader, ProcessRunner.MaxOutputChars);
+
+        Assert.Equal(ProcessRunner.MaxOutputChars, text.Length);
+        Assert.Equal(-1, reader.Peek()); // everything was read, so a process never blocks on a full pipe
+    }
+
+    [Fact]
+    public async Task A_process_is_killed_when_the_agent_stops()
+    {
+        if (!OperatingSystem.IsWindows())
+        {
+            return;
+        }
+
+        var marker = Path.Combine(Path.GetTempPath(), "sqlinsight-kill-" + Guid.NewGuid().ToString("N") + ".txt");
+        var powershell = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.System), @"WindowsPowerShell\v1.0\powershell.exe");
+        using var stop = new CancellationTokenSource(TimeSpan.FromMinutes(2));
+        var started = Stopwatch.StartNew();
+        try
+        {
+            // Writes its process id, then would wait for a minute; the agent stops as soon as it has started.
+            var stopWhenStarted = Task.Run(async () =>
+            {
+                while (!File.Exists(marker) && started.Elapsed < TimeSpan.FromMinutes(1))
+                {
+                    await Task.Delay(100);
+                }
+
+                await Task.Delay(500);
+                await stop.CancelAsync();
+            });
+            await Assert.ThrowsAnyAsync<OperationCanceledException>(() => new ProcessRunner().RunAsync(powershell,
+                ["-NoProfile", "-NonInteractive", "-Command", $"Set-Content -LiteralPath '{marker}' -Value $PID; Start-Sleep -Seconds 60"],
+                new Dictionary<string, string>(), TimeSpan.FromMinutes(5), stop.Token));
+            await stopWhenStarted;
+            Assert.True(started.Elapsed < TimeSpan.FromSeconds(55), $"The process was waited for ({started.Elapsed}).");
+
+            var pid = int.Parse((await File.ReadAllTextAsync(marker)).Trim(), System.Globalization.CultureInfo.InvariantCulture);
+            var gone = Stopwatch.StartNew();
+            while (IsRunning(pid))
+            {
+                Assert.True(gone.Elapsed < TimeSpan.FromSeconds(10), $"PowerShell ({pid}) was left running.");
+                await Task.Delay(100);
+            }
+        }
+        finally
+        {
+            File.Delete(marker);
+        }
+
+        static bool IsRunning(int pid)
+        {
+            try
+            {
+                using var process = Process.GetProcessById(pid);
+                return !process.HasExited;
+            }
+            catch (ArgumentException)
+            {
+                return false;
+            }
+        }
+    }
+}

tests/SQLInsight.Servers.Tests/SyntheticCheckRunnerTests.cs

@@ -221,6 +221,39 @@ public sealed class SyntheticCheckRunnerTests : IDisposable
         Assert.Contains("The database reported an error", error.Message, StringComparison.Ordinal);
     }
 
+    /// <summary>Promises a long page, sends a little of it and hangs up.</summary>
+    private void ServeBrokenBody() => _ = Task.Run(async () =>
+    {
+        using var client = await _listener.AcceptTcpClientAsync(_stop.Token);
+        var stream = client.GetStream();
+        var buffer = new byte[8192];
+        _ = await stream.ReadAsync(buffer);
+        await stream.WriteAsync(Encoding.ASCII.GetBytes("HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: 100000\r\n\r\n<html>begynd"));
+        await stream.FlushAsync();
+        client.Client.Shutdown(SocketShutdown.Both);
+    });
+
+    [Fact]
+    public async Task A_page_that_breaks_off_while_it_is_read_is_a_failed_check_not_an_error()
+    {
+        ServeBrokenBody();
+
+        var result = await Runner().RunAsync(Run(SyntheticCheckKind.Http, new() { Url = new Uri($"http://127.0.0.1:{Port}/"), ExpectedText = "slut" }), CancellationToken.None);
+
+        Assert.False(result.Success);
+        Assert.Contains("afbrudt", result.Message, StringComparison.Ordinal);
+        Assert.True(Assert.Single(result.Details, d => d.Name == "Adresse").Sensitive); // a URL may carry a secret in its query
+    }
+
+    [Fact]
+    public void A_run_never_shows_its_password()
+    {
+        var run = new SyntheticCheckRun(SyntheticCheckKind.Sql, new() { Server = "sql01" }, TimeSpan.FromSeconds(5), "Hemmelig-123");
+
+        Assert.DoesNotContain("Hemmelig-123", run.ToString(), StringComparison.Ordinal);
+        Assert.Contains("Password = ***", run.ToString(), StringComparison.Ordinal);
+    }
+
     [Fact]
     public async Task A_query_that_writes_is_never_run_even_if_it_was_stored()
     {