diff --git a/code-reviews/ExternalSystemGateway/findings.md b/code-reviews/ExternalSystemGateway/findings.md index cc7c050..67384ba 100644 --- a/code-reviews/ExternalSystemGateway/findings.md +++ b/code-reviews/ExternalSystemGateway/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 11 | +| Open findings | 4 | ## Summary @@ -215,7 +215,7 @@ is flipped back to `true`. |--|--| | Severity | Medium | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:114-115`, `src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs:86-87` | **Description** @@ -242,7 +242,21 @@ should be tracked against the SiteRuntime module. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). `CachedCallAsync` and `CachedWriteAsync` now pass +the definition's `MaxRetries` to `EnqueueAsync` verbatim — the `> 0` guard is dropped, so +a legitimately-configured `MaxRetries` of 0 ("never retry") is honoured instead of being +collapsed to the S&F default. The `RetryDelay > TimeSpan.Zero` guard is deliberately +**kept**: `TimeSpan.Zero` is the entity default for an unconfigured field and a literal +zero-delay retry loop is not a valid configuration, so falling back to the S&F default +interval for an unset delay is correct (only `MaxRetries == 0` is a meaningful operator +choice). Regression test `CachedCall_TransientFailure_ZeroMaxRetriesIsHonouredNotTreatedAsUnset` +buffers a transient failure and asserts the buffered message carries `MaxRetries == 0` +rather than the S&F default; `CachedCall_TransientFailure_BuffersWithSystemRetrySettings` +additionally covers a non-default settings pass-through. The companion fix in +`SiteExternalSystemRepository.MapExternalSystem` to actually read the `MaxRetries` / +`RetryDelay` columns from SQLite remains a tracked follow-up against the SiteRuntime +module (outside this module's edit scope) — until then, sites still supply +`MaxRetries == 0`, which this fix now correctly honours as "never retry". ### ExternalSystemGateway-005 — `HttpRequestMessage` and `HttpResponseMessage` are not disposed @@ -250,7 +264,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:133-167` | **Description** @@ -272,15 +286,22 @@ occurs on the exception paths. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). `InvokeHttpAsync` now declares the request as +`using var request` and wraps all response handling in a `using (response)` block, so +both `IDisposable` instances (and the request's `StringContent` / the response content +stream) are released on the success path **and** on the permanent/transient +exception paths. Regression tests `Call_SuccessfulHttp_DisposesRequestAndResponse` and +`Call_PermanentFailure_StillDisposesRequestAndResponse` use a disposal-tracking +`HttpMessageHandler`/`HttpContent` and assert both the request and the response content +are disposed; both were verified to fail before the `using` wrappers were added. ### ExternalSystemGateway-006 — `BuildUrl` ignores path templates and appends a trailing slash for empty paths | | | |--|--| -| Severity | Medium | +| Severity | Medium — partially re-triaged: trailing-slash bug fixed; path-templating sub-issue is a design decision (see Resolution) | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:180-196` | **Description** @@ -305,7 +326,25 @@ example and state that paths are literal. Also avoid appending a trailing `/` wh **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). The **trailing-slash bug** is fixed: `BuildUrl` +now appends a `/`-joined path segment only when the method's path is non-empty after +trimming, so a method targeting the base URL itself produces `https://host/api` rather +than `https://host/api/`. Regression tests `Call_MethodWithEmptyPath_DoesNotAppendTrailingSlash` +and `Call_MethodWithPath_BuildsExpectedUrl` (asserting on the captured request URI) +cover the empty-path and normal-path cases; the empty-path test was verified to fail +before the fix. + +Re-triage of the **path-templating sub-issue** (`{id}` placeholder substitution): this +is a genuine design decision, not a code bug, and it requires editing the component +design doc — both outside this module's edit scope (`src/`, `tests/`, this file only). +The current code treats method paths as literal strings and routes parameters to the +query string (GET/DELETE) or JSON body (POST/PUT); a method authored as `/recipes/{id}` +sends the `{id}` token verbatim. **Tracked follow-up / surfaced design question:** the +design owner must decide whether path templating is in scope — if yes, implement +`{name}` substitution in `BuildUrl` and exclude substituted params from the +query/body; if no, the `Component-ExternalSystemGateway.md` `/recipes/{id}` example must +be changed to a literal path. The trailing-slash defect (the concrete correctness bug +in this finding) is fully resolved. ### ExternalSystemGateway-007 — External error response bodies are echoed verbatim into script-visible error messages @@ -313,7 +352,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:167-177` | **Description** @@ -335,15 +374,25 @@ the script. Optionally only include the body when the content type is textual. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). `InvokeHttpAsync` now truncates the external error +response body to `MaxErrorBodyChars` (2048) via a `Truncate` helper before embedding it +into the transient/permanent exception message — so a misbehaving or hostile endpoint +can no longer inflate every script-visible `ErrorMessage` and Site Event Logging entry +with a multi-megabyte body. When truncation occurs the message is suffixed with +`… [truncated, N chars total]` so the original size is still visible. Regression test +`Call_PermanentFailureWithHugeErrorBody_TruncatesErrorMessage` drives a 400 with a +500 KB body and asserts the resulting `ErrorMessage` is bounded (< 4096 chars); it was +verified to fail (500 040-char message) before the cap was added. Content-type +filtering was considered optional in the recommendation and was not implemented — the +size cap alone closes the inflation/disclosure vector. ### ExternalSystemGateway-008 — Cancellation is conflated with transient timeout failure | | | |--|--| -| Severity | Medium | +| Severity | Medium — re-triaged: root cause already fixed in current source (see Resolution) | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ErrorClassifier.cs:24-30`, `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:157-159` | **Description** @@ -367,15 +416,37 @@ classification. Only treat a cancellation as a timeout when the supplied token i **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). **Re-triage:** the root cause described — a +caller-initiated cancellation being misclassified as a transient failure — is **no +longer present in the current source** and is not reproducible. `InvokeHttpAsync` +already wraps both `SendAsync` and the response-body `ReadAsStringAsync` in ordered +`catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)` +filters that rethrow the caller's cancellation *before* the +`catch (Exception ex) when (ErrorClassifier.IsTransient(ex))` branch is ever reached +(this was added alongside the `ExternalSystemGateway-002` timeout fix). A caller-cancel +therefore propagates as `OperationCanceledException` and is never buffered; only the +gateway's own timeout token reclassifies as transient. + +`ErrorClassifier.IsTransient(Exception)` does still return `true` for +`TaskCanceledException`/`OperationCanceledException`, but that is **correct and +intentional**: a `TaskCanceledException` raised by an HTTP timeout *is* a genuine +transient failure, and the only caller (`InvokeHttpAsync:238`) is unreachable for a +caller-cancellation because the two preceding `when`-filtered catches intercept it +first. The transient-vs-cancel decision is contextual (which token fired) and cannot +be made from the exception type alone, which is exactly why the call site does it. +No source change was required. A regression guard, +`CachedCall_CallerCancellation_IsNotBufferedAsTransient`, was added: it cancels the +caller token mid-`CachedCall` and asserts an `OperationCanceledException` is thrown and +the S&F buffer remains empty (the cancelled work is not retried). The existing +`Call_CallerCancellation_IsNotMisreportedAsTimeout` covers the synchronous `Call` path. ### ExternalSystemGateway-009 — `StoreAndForwardResult` from `EnqueueAsync` is discarded; permanent failures during buffering are swallowed | | | |--|--| -| Severity | Medium | +| Severity | Medium — re-triaged: root cause subsumed by the ExternalSystemGateway-003 dispatch redesign (see Resolution) | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:109-117` | **Description** @@ -398,7 +469,27 @@ partly subsumed by the dispatch redesign in finding 003.) **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). **Re-triage:** the stated root cause — "a +permanent failure surfaced by `EnqueueAsync`'s immediate-delivery attempt is silently +lost" — **can no longer occur** in the current source, and the dead `sfResult` variable +the finding cites has already been removed. The `ExternalSystemGateway-003` fix changed +`CachedCallAsync` to call `EnqueueAsync` with `attemptImmediateDelivery: false`. With +that flag, `EnqueueAsync` never invokes the registered delivery handler: it skips the +immediate-delivery block entirely (so the `StoreAndForwardResult(false, …, …)` +permanent-failure return at `StoreAndForwardService.cs:147` is unreachable from this +caller) and unconditionally buffers, returning `Accepted: true, WasBuffered: true` +(`StoreAndForwardService.cs:180`). The `ExternalCallResult(true, null, null, +WasBuffered: true)` that `CachedCallAsync` returns is therefore now factually correct +in every reachable case — the message *is* buffered and there is no swallowed permanent +failure. Permanent HTTP 4xx failures are still surfaced synchronously, because +`CachedCallAsync` makes its own first HTTP attempt and catches +`PermanentExternalSystemException` *before* it ever reaches `EnqueueAsync`. No source +change was required beyond the `ExternalSystemGateway-003` redesign that already landed. +Coverage: `CachedCall_TransientFailure_BuffersWithSystemRetrySettings` asserts both +`result.WasBuffered == true` and that the message is genuinely present in the S&F buffer +(depth == 1), confirming the `WasBuffered: true` claim is not a lie; the existing +`CachedCall` permanent-failure path is exercised by `Call_Permanent400_ReturnsPermanentError` +semantics shared via `InvokeHttpAsync`. ### ExternalSystemGateway-010 — `GetConnectionAsync` leaks the `SqlConnection` when `OpenAsync` fails @@ -406,7 +497,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs:48-50` | **Description** @@ -427,7 +518,14 @@ Wrap the open in a try/catch that disposes the connection before rethrowing: **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). `GetConnectionAsync` now wraps `OpenAsync` in a +`try/catch` that calls `await connection.DisposeAsync()` before rethrowing, so a failed +open (unreachable server, bad credentials, cancellation) no longer leaks the +`SqlConnection`. Connection creation was extracted into an `internal virtual +CreateConnection(string)` factory so the failure path is unit-testable. Regression test +`GetConnection_OpenFails_DisposesConnectionBeforeRethrowing` substitutes a `DbConnection` +whose `OpenAsync` always throws and asserts the connection is disposed when the +exception propagates; it was verified to fail before the `try/catch` was added. ### ExternalSystemGateway-011 — Every call performs a full repository scan of all systems and methods diff --git a/src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs b/src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs index dcb30f2..e88d0f5 100644 --- a/src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs +++ b/src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs @@ -45,11 +45,29 @@ public class DatabaseGateway : IDatabaseGateway throw new InvalidOperationException($"Database connection '{connectionName}' not found"); } - var connection = new SqlConnection(definition.ConnectionString); - await connection.OpenAsync(cancellationToken); + var connection = CreateConnection(definition.ConnectionString); + try + { + await connection.OpenAsync(cancellationToken); + } + catch + { + // OpenAsync failed (unreachable server, bad credentials, cancellation) — + // dispose the just-created connection before the exception propagates so + // it is not leaked (ExternalSystemGateway-010). + await connection.DisposeAsync(); + throw; + } return connection; } + /// + /// Creates the underlying ADO.NET connection for a connection string. Virtual so + /// tests can substitute a connection whose OpenAsync fails. + /// + internal virtual DbConnection CreateConnection(string connectionString) => + new SqlConnection(connectionString); + /// /// Submits a SQL write to the store-and-forward engine for reliable delivery. /// @@ -78,12 +96,15 @@ public class DatabaseGateway : IDatabaseGateway Parameters = parameters }); + // The per-connection retry settings are passed through verbatim — a + // configured MaxRetries of 0 means "never retry" and must NOT be + // collapsed to the S&F default (ExternalSystemGateway-004). await _storeAndForward.EnqueueAsync( StoreAndForwardCategory.CachedDbWrite, connectionName, payload, originInstanceName, - definition.MaxRetries > 0 ? definition.MaxRetries : null, + definition.MaxRetries, definition.RetryDelay > TimeSpan.Zero ? definition.RetryDelay : null); } diff --git a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs index 2408927..52abe8a 100644 --- a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs +++ b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs @@ -113,12 +113,16 @@ public class ExternalSystemClient : IExternalSystemClient // attemptImmediateDelivery: false — this method already made the HTTP // attempt above; letting EnqueueAsync re-invoke the handler would // dispatch the same request a second time. + // + // The per-system retry settings are passed through verbatim — a + // configured MaxRetries of 0 means "never retry" and must NOT be + // collapsed to the S&F default (ExternalSystemGateway-004). await _storeAndForward.EnqueueAsync( StoreAndForwardCategory.ExternalSystem, systemName, payload, originInstanceName, - system.MaxRetries > 0 ? system.MaxRetries : null, + system.MaxRetries, system.RetryDelay > TimeSpan.Zero ? system.RetryDelay : null, attemptImmediateDelivery: false); @@ -183,7 +187,11 @@ public class ExternalSystemClient : IExternalSystemClient var client = _httpClientFactory.CreateClient($"ExternalSystem_{system.Name}"); var url = BuildUrl(system.EndpointUrl, method.Path, parameters, method.HttpMethod); - var request = new HttpRequestMessage(new HttpMethod(method.HttpMethod), url); + + // The request and response own IDisposable resources (StringContent, the + // response content stream). Dispose both, including on the exception paths + // (ExternalSystemGateway-005). + using var request = new HttpRequestMessage(new HttpMethod(method.HttpMethod), url); // Apply authentication ApplyAuth(request, system); @@ -232,44 +240,75 @@ public class ExternalSystemClient : IExternalSystemClient throw ErrorClassifier.AsTransient($"Connection error to {system.Name}: {ex.Message}", ex); } - // The timeout also covers reading the response body (the design's - // "round-trip" guarantee), so the linked token is used for the read too. - string body; - try + using (response) { - body = await response.Content.ReadAsStringAsync(linkedCts.Token); + // The timeout also covers reading the response body (the design's + // "round-trip" guarantee), so the linked token is used for the read too. + string body; + try + { + body = await response.Content.ReadAsStringAsync(linkedCts.Token); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; + } + catch (OperationCanceledException ex) when (timeoutCts.IsCancellationRequested) + { + throw ErrorClassifier.AsTransient( + $"Timeout reading response from {system.Name} after {_options.DefaultHttpTimeout.TotalSeconds:0.##}s", ex); + } + + if (response.IsSuccessStatusCode) + { + return body; + } + + // Bound the external error body before embedding it into a + // script-visible message / event-log entry — a misbehaving or hostile + // endpoint must not be able to inflate every error string + // (ExternalSystemGateway-007). + var errorBody = Truncate(body, MaxErrorBodyChars); + + if (ErrorClassifier.IsTransient(response.StatusCode)) + { + throw ErrorClassifier.AsTransient( + $"HTTP {(int)response.StatusCode} from {system.Name}: {errorBody}"); + } + + throw new PermanentExternalSystemException( + $"HTTP {(int)response.StatusCode} from {system.Name}: {errorBody}", + (int)response.StatusCode); } - catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + } + + /// + /// Upper bound (characters) on an external error response body echoed into a + /// script-visible error message — see ExternalSystemGateway-007. + /// + private const int MaxErrorBodyChars = 2048; + + private static string Truncate(string value, int maxChars) + { + if (string.IsNullOrEmpty(value) || value.Length <= maxChars) { - throw; - } - catch (OperationCanceledException ex) when (timeoutCts.IsCancellationRequested) - { - throw ErrorClassifier.AsTransient( - $"Timeout reading response from {system.Name} after {_options.DefaultHttpTimeout.TotalSeconds:0.##}s", ex); + return value; } - if (response.IsSuccessStatusCode) - { - return body; - } - - var errorBody = body; - - if (ErrorClassifier.IsTransient(response.StatusCode)) - { - throw ErrorClassifier.AsTransient( - $"HTTP {(int)response.StatusCode} from {system.Name}: {errorBody}"); - } - - throw new PermanentExternalSystemException( - $"HTTP {(int)response.StatusCode} from {system.Name}: {errorBody}", - (int)response.StatusCode); + return value.Substring(0, maxChars) + $"… [truncated, {value.Length} chars total]"; } private static string BuildUrl(string baseUrl, string path, IReadOnlyDictionary? parameters, string httpMethod) { - var url = baseUrl.TrimEnd('/') + "/" + path.TrimStart('/'); + // A method that targets the base URL itself has an empty (or "/") path. + // Appending a trailing "/" in that case yields ".../api/" which some + // servers treat as a distinct resource — only append a segment when the + // method actually defines a non-empty relative path (ExternalSystemGateway-006). + var trimmedBase = baseUrl.TrimEnd('/'); + var trimmedPath = path.Trim().TrimStart('/'); + var url = string.IsNullOrEmpty(trimmedPath) + ? trimmedBase + : trimmedBase + "/" + trimmedPath; // For GET/DELETE, append parameters as query string if ((httpMethod.Equals("GET", StringComparison.OrdinalIgnoreCase) || diff --git a/tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs b/tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs index 0d9cd27..7f1fc94 100644 --- a/tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs +++ b/tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs @@ -1,3 +1,5 @@ +using System.Data; +using System.Data.Common; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using ScadaLink.Commons.Entities.ExternalSystems; @@ -75,4 +77,66 @@ public class DatabaseGatewayTests Assert.False(delivered); // permanent — the S&F engine parks the message } + + // ── ExternalSystemGateway-010: SqlConnection must not leak when OpenAsync fails ── + + [Fact] + public async Task GetConnection_OpenFails_DisposesConnectionBeforeRethrowing() + { + var conn = new DatabaseConnectionDefinition("testDb", "Server=localhost;Database=test") { Id = 1 }; + _repository.GetAllDatabaseConnectionsAsync().Returns(new List { conn }); + + var fake = new ThrowingDbConnection(); + var gateway = new ConnectionFactoryStubGateway(_repository, fake); + + await Assert.ThrowsAsync( + () => gateway.GetConnectionAsync("testDb")); + + Assert.True(fake.WasDisposed, "The SqlConnection was leaked — it must be disposed when OpenAsync fails"); + } + + /// Test gateway that substitutes the connection factory with a stub. + private sealed class ConnectionFactoryStubGateway : DatabaseGateway + { + private readonly DbConnection _connection; + + public ConnectionFactoryStubGateway(IExternalSystemRepository repository, DbConnection connection) + : base(repository, NullLogger.Instance) => _connection = connection; + + internal override DbConnection CreateConnection(string connectionString) => _connection; + } + + /// A DbConnection whose OpenAsync always fails, tracking whether it was disposed. + private sealed class ThrowingDbConnection : DbConnection + { + public bool WasDisposed { get; private set; } + + public override Task OpenAsync(CancellationToken cancellationToken) => + throw new InvalidOperationException("simulated open failure"); + public override void Open() => throw new InvalidOperationException("simulated open failure"); + + protected override void Dispose(bool disposing) + { + if (disposing) WasDisposed = true; + base.Dispose(disposing); + } + + public override ValueTask DisposeAsync() + { + WasDisposed = true; + return base.DisposeAsync(); + } + + // Unused abstract members. + [System.Diagnostics.CodeAnalysis.AllowNull] + public override string ConnectionString { get; set; } = string.Empty; + public override string Database => string.Empty; + public override string DataSource => string.Empty; + public override string ServerVersion => string.Empty; + public override ConnectionState State => ConnectionState.Closed; + public override void ChangeDatabase(string databaseName) => throw new NotSupportedException(); + public override void Close() { } + protected override DbTransaction BeginDbTransaction(IsolationLevel il) => throw new NotSupportedException(); + protected override DbCommand CreateDbCommand() => throw new NotSupportedException(); + } } diff --git a/tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs b/tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs index 7d8f23b..83b25c8 100644 --- a/tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs +++ b/tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs @@ -328,6 +328,281 @@ public class ExternalSystemClientTests () => client.CallAsync("TestAPI", "getData", cancellationToken: cts.Token)); } + // ── ExternalSystemGateway-004: per-system retry settings honoured for cached calls ── + + [Fact] + public async Task CachedCall_TransientFailure_BuffersWithSystemRetrySettings() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") + { + Id = 1, + MaxRetries = 7, + RetryDelay = TimeSpan.FromSeconds(42), + }; + var method = new ExternalSystemMethod("postData", "POST", "/post") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var httpClient = new HttpClient(new MockHttpMessageHandler(HttpStatusCode.InternalServerError, "boom")); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var dbName = $"EsgRetry_{Guid.NewGuid():N}"; + var connStr = $"Data Source={dbName};Mode=Memory;Cache=Shared"; + using var keepAlive = new SqliteConnection(connStr); + keepAlive.Open(); + var storage = new StoreAndForwardStorage(connStr, NullLogger.Instance); + await storage.InitializeAsync(); + // S&F defaults deliberately different from the system's settings. + var sfOptions = new StoreAndForwardOptions + { + DefaultMaxRetries = 3, + DefaultRetryInterval = TimeSpan.FromMinutes(10), + RetryTimerInterval = TimeSpan.FromMinutes(10), + }; + var sf = new StoreAndForwardService(storage, sfOptions, NullLogger.Instance); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance, + storeAndForward: sf); + + var result = await client.CachedCallAsync("TestAPI", "postData"); + Assert.True(result.WasBuffered); + + var depth = await storage.GetBufferDepthByCategoryAsync(); + Assert.Equal(1, depth[ScadaLink.Commons.Types.Enums.StoreAndForwardCategory.ExternalSystem]); + + var (maxRetries, retryIntervalMs) = ReadBufferedRetrySettings(connStr); + Assert.Equal(7, maxRetries); + Assert.Equal((long)TimeSpan.FromSeconds(42).TotalMilliseconds, retryIntervalMs); + } + + private static (int MaxRetries, long RetryIntervalMs) ReadBufferedRetrySettings(string connStr) + { + using var conn = new SqliteConnection(connStr); + conn.Open(); + using var cmd = conn.CreateCommand(); + cmd.CommandText = "SELECT max_retries, retry_interval_ms FROM sf_messages"; + using var reader = cmd.ExecuteReader(); + Assert.True(reader.Read(), "expected exactly one buffered message"); + var result = (reader.GetInt32(0), reader.GetInt64(1)); + Assert.False(reader.Read(), "expected exactly one buffered message"); + return result; + } + + [Fact] + public async Task CachedCall_TransientFailure_ZeroMaxRetriesIsHonouredNotTreatedAsUnset() + { + // MaxRetries == 0 must mean "never retry", not "fall back to the S&F default". + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") + { + Id = 1, + MaxRetries = 0, + RetryDelay = TimeSpan.FromSeconds(5), + }; + var method = new ExternalSystemMethod("postData", "POST", "/post") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var httpClient = new HttpClient(new MockHttpMessageHandler(HttpStatusCode.InternalServerError, "boom")); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var dbName = $"EsgRetryZero_{Guid.NewGuid():N}"; + var connStr = $"Data Source={dbName};Mode=Memory;Cache=Shared"; + using var keepAlive = new SqliteConnection(connStr); + keepAlive.Open(); + var storage = new StoreAndForwardStorage(connStr, NullLogger.Instance); + await storage.InitializeAsync(); + var sfOptions = new StoreAndForwardOptions + { + DefaultMaxRetries = 99, + DefaultRetryInterval = TimeSpan.FromMinutes(10), + RetryTimerInterval = TimeSpan.FromMinutes(10), + }; + var sf = new StoreAndForwardService(storage, sfOptions, NullLogger.Instance); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance, + storeAndForward: sf); + + await client.CachedCallAsync("TestAPI", "postData"); + + var (maxRetries, _) = ReadBufferedRetrySettings(connStr); + Assert.Equal(0, maxRetries); // honoured — not the default of 99 + } + + // ── ExternalSystemGateway-005: HttpRequestMessage / HttpResponseMessage disposal ── + + [Fact] + public async Task Call_SuccessfulHttp_DisposesRequestAndResponse() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("getData", "GET", "/data") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new DisposalTrackingHandler(HttpStatusCode.OK, "{\"ok\":true}"); + var httpClient = new HttpClient(handler); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "getData"); + + Assert.True(handler.RequestDisposed, "HttpRequestMessage was not disposed"); + Assert.True(handler.ResponseContentDisposed, "HttpResponseMessage content was not disposed"); + } + + [Fact] + public async Task Call_PermanentFailure_StillDisposesRequestAndResponse() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("badMethod", "POST", "/bad") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new DisposalTrackingHandler(HttpStatusCode.BadRequest, "bad request"); + var httpClient = new HttpClient(handler); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "badMethod"); + + Assert.True(handler.RequestDisposed, "HttpRequestMessage was not disposed on the error path"); + Assert.True(handler.ResponseContentDisposed, "HttpResponseMessage content was not disposed on the error path"); + } + + // ── ExternalSystemGateway-006: BuildUrl — empty path must not append a trailing slash ── + + [Fact] + public async Task Call_MethodWithEmptyPath_DoesNotAppendTrailingSlash() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com/api", "none") { Id = 1 }; + var method = new ExternalSystemMethod("root", "GET", "") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new RequestCapturingHandler(HttpStatusCode.OK, "{}"); + var httpClient = new HttpClient(handler); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "root"); + + Assert.Equal("https://api.example.com/api", handler.LastUri!.ToString()); + } + + [Fact] + public async Task Call_MethodWithPath_BuildsExpectedUrl() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com/api", "none") { Id = 1 }; + var method = new ExternalSystemMethod("getData", "GET", "/data") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new RequestCapturingHandler(HttpStatusCode.OK, "{}"); + var httpClient = new HttpClient(handler); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "getData"); + + Assert.Equal("https://api.example.com/api/data", handler.LastUri!.ToString()); + } + + // ── ExternalSystemGateway-007: external error body must be truncated, not echoed verbatim ── + + [Fact] + public async Task Call_PermanentFailureWithHugeErrorBody_TruncatesErrorMessage() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("badMethod", "POST", "/bad") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var hugeBody = new string('X', 500_000); + var handler = new MockHttpMessageHandler(HttpStatusCode.BadRequest, hugeBody); + var httpClient = new HttpClient(handler); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + var result = await client.CallAsync("TestAPI", "badMethod"); + + Assert.False(result.Success); + // The error message must be bounded — a misbehaving endpoint cannot inflate + // every script-visible error string / event-log entry. + Assert.True(result.ErrorMessage!.Length < 4096, + $"Error message was {result.ErrorMessage.Length} chars — expected it to be truncated"); + } + + // ── ExternalSystemGateway-008: cancellation of a CachedCall must not be buffered ── + + [Fact] + public async Task CachedCall_CallerCancellation_IsNotBufferedAsTransient() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("postData", "POST", "/post") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var httpClient = new HttpClient(new HangingHttpMessageHandler(TimeSpan.FromMinutes(10))); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var dbName = $"EsgCancel_{Guid.NewGuid():N}"; + var connStr = $"Data Source={dbName};Mode=Memory;Cache=Shared"; + using var keepAlive = new SqliteConnection(connStr); + keepAlive.Open(); + var storage = new StoreAndForwardStorage(connStr, NullLogger.Instance); + await storage.InitializeAsync(); + var sfOptions = new StoreAndForwardOptions + { + DefaultRetryInterval = TimeSpan.FromMinutes(10), + RetryTimerInterval = TimeSpan.FromMinutes(10), + }; + var sf = new StoreAndForwardService(storage, sfOptions, NullLogger.Instance); + + var options = new ExternalSystemGatewayOptions { DefaultHttpTimeout = TimeSpan.FromMinutes(5) }; + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance, + storeAndForward: sf, + options: Microsoft.Extensions.Options.Options.Create(options)); + + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(200)); + + // Caller asked to abandon the work — it must NOT be buffered for retry. + await Assert.ThrowsAnyAsync( + () => client.CachedCallAsync("TestAPI", "postData", cancellationToken: cts.Token)); + + var depth = await storage.GetBufferDepthByCategoryAsync(); + Assert.False( + depth.TryGetValue(ScadaLink.Commons.Types.Enums.StoreAndForwardCategory.ExternalSystem, out var n) && n > 0, + "A caller-cancelled CachedCall must not be buffered for retry"); + } + /// /// Test helper: mock HTTP message handler. /// @@ -351,6 +626,72 @@ public class ExternalSystemClientTests } } + /// + /// Test helper: tracks disposal of the request and the response content. + /// + private class DisposalTrackingHandler : HttpMessageHandler + { + private readonly HttpStatusCode _statusCode; + private readonly string _body; + + public DisposalTrackingHandler(HttpStatusCode statusCode, string body) + { + _statusCode = statusCode; + _body = body; + } + + public bool RequestDisposed { get; private set; } + public bool ResponseContentDisposed { get; private set; } + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + request.Content = new TrackingContent(string.Empty, () => RequestDisposed = true); + var response = new HttpResponseMessage(_statusCode) + { + Content = new TrackingContent(_body, () => ResponseContentDisposed = true) + }; + return Task.FromResult(response); + } + + private sealed class TrackingContent : StringContent + { + private readonly Action _onDispose; + public TrackingContent(string content, Action onDispose) : base(content) => _onDispose = onDispose; + + protected override void Dispose(bool disposing) + { + if (disposing) _onDispose(); + base.Dispose(disposing); + } + } + } + + /// + /// Test helper: captures the request URI of the last request. + /// + private class RequestCapturingHandler : HttpMessageHandler + { + private readonly HttpStatusCode _statusCode; + private readonly string _body; + + public RequestCapturingHandler(HttpStatusCode statusCode, string body) + { + _statusCode = statusCode; + _body = body; + } + + public Uri? LastUri { get; private set; } + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + LastUri = request.RequestUri; + return Task.FromResult(new HttpResponseMessage(_statusCode) + { + Content = new StringContent(_body) + }); + } + } + /// /// Test helper: an HTTP handler that hangs until cancelled (simulates a slow/hung system). ///