From f980bd3a4a04d4005056e36543100f5172f25512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Eggert=20Lundsteen=20Olsen?= Date: Fri, 2 Oct 2026 15:56:17 +0200 Subject: [PATCH] Review 1.5: synthetic check runner: network surprises are results, AX process never outlives the agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- .../Synthetic/AxBusinessConnector.cs | 59 ++++++++++++-- .../Synthetic/SyntheticCheckRunner.cs | 39 ++++++++-- .../ProcessRunnerTests.cs | 77 +++++++++++++++++++ .../SyntheticCheckRunnerTests.cs | 33 ++++++++ 4 files changed, 198 insertions(+), 10 deletions(-) create mode 100644 tests/SQLInsight.Servers.Tests/ProcessRunnerTests.cs diff --git a/src/SQLInsight.Servers/Synthetic/AxBusinessConnector.cs b/src/SQLInsight.Servers/Synthetic/AxBusinessConnector.cs index ec3c897..67f21cf 100644 --- a/src/SQLInsight.Servers/Synthetic/AxBusinessConnector.cs +++ b/src/SQLInsight.Servers/Synthetic/AxBusinessConnector.cs @@ -64,9 +64,14 @@ public interface IProcessRunner CancellationToken cancellationToken); } -/// Runs a process and kills it (with its children) when it takes too long. +/// +/// Runs a process and kills it (with its children) when it takes too long or the agent stops; never leaves one behind. +/// At most of its output and of its errors are kept. +/// public sealed class ProcessRunner : IProcessRunner { + public const int MaxOutputChars = 64 * 1024; + public async Task RunAsync(string fileName, IReadOnlyList arguments, IReadOnlyDictionary 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 error = process.StandardError.ReadToEndAsync(cancellationToken); + var output = ReadBoundedAsync(process.StandardOutput, MaxOutputChars); + 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. + } + } + + /// A reader no longer waited for ends when the process does; its error (the stream was closed) is not left unobserved. + private static void Observe(Task task) => + _ = task.ContinueWith(static t => _ = t.Exception, CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + + /// Reads to the end, keeping at most : the rest is read and dropped, so the process never blocks on a full pipe. + internal static async Task 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(); } } diff --git a/src/SQLInsight.Servers/Synthetic/SyntheticCheckRunner.cs b/src/SQLInsight.Servers/Synthetic/SyntheticCheckRunner.cs index 2cd400e..b5485cf 100644 --- a/src/SQLInsight.Servers/Synthetic/SyntheticCheckRunner.cs +++ b/src/SQLInsight.Servers/Synthetic/SyntheticCheckRunner.cs @@ -14,7 +14,15 @@ using SQLInsight.Core.SyntheticChecks; namespace SQLInsight.Servers.Synthetic; /// One run of a check: its settings, the opened SQL login password (if any) and how long it may take. -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) +{ + /// The password never ends up in a log or an exception through ToString. + private bool PrintMembers(StringBuilder builder) + { + builder.Append(CultureInfo.InvariantCulture, $"Kind = {Kind}, Settings = {Settings}, Timeout = {Timeout}, Password = {(Password is null ? "null" : "***")}"); + return true; + } +} /// /// 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 { new("Adresse", url.ToString()) }; + var details = new List { 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)); diff --git a/tests/SQLInsight.Servers.Tests/ProcessRunnerTests.cs b/tests/SQLInsight.Servers.Tests/ProcessRunnerTests.cs new file mode 100644 index 0000000..8c190b6 --- /dev/null +++ b/tests/SQLInsight.Servers.Tests/ProcessRunnerTests.cs @@ -0,0 +1,77 @@ +using System.Diagnostics; +using SQLInsight.Servers.Synthetic; + +namespace SQLInsight.Servers.Tests; + +/// The process the AX logon runs in: its output is bounded, and it never outlives the agent. +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(() => new ProcessRunner().RunAsync(powershell, + ["-NoProfile", "-NonInteractive", "-Command", $"Set-Content -LiteralPath '{marker}' -Value $PID; Start-Sleep -Seconds 60"], + new Dictionary(), 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; + } + } + } +} diff --git a/tests/SQLInsight.Servers.Tests/SyntheticCheckRunnerTests.cs b/tests/SQLInsight.Servers.Tests/SyntheticCheckRunnerTests.cs index 1137f44..802f34c 100644 --- a/tests/SQLInsight.Servers.Tests/SyntheticCheckRunnerTests.cs +++ b/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); } + /// Promises a long page, sends a little of it and hangs up. + 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\nbegynd")); + 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() {