From da955042aa32ccae59e7a37c65d3241c6a3db994 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 21:22:01 -0400 Subject: [PATCH] =?UTF-8?q?fix(inbound-api):=20resolve=20InboundAPI-002,00?= =?UTF-8?q?4,006,008=20=E2=80=94=20disconnect=20vs=20timeout,=20body=20siz?= =?UTF-8?q?e=20limit,=20active-node=20gate;=20surface=20InboundAPI-007?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/InboundAPI/findings.md | 88 +++++++++-- .../EndpointExtensions.cs | 13 +- src/ScadaLink.InboundAPI/IActiveNodeGate.cs | 24 +++ .../InboundApiEndpointFilter.cs | 78 ++++++++++ src/ScadaLink.InboundAPI/InboundApiOptions.cs | 14 ++ .../InboundScriptExecutor.cs | 26 +++- .../ServiceCollectionExtensions.cs | 4 + .../EndpointGatingTests.cs | 147 ++++++++++++++++++ .../InboundScriptExecutorTests.cs | 84 ++++++++++ .../ScadaLink.InboundAPI.Tests.csproj | 4 + 10 files changed, 462 insertions(+), 20 deletions(-) create mode 100644 src/ScadaLink.InboundAPI/IActiveNodeGate.cs create mode 100644 src/ScadaLink.InboundAPI/InboundApiEndpointFilter.cs create mode 100644 tests/ScadaLink.InboundAPI.Tests/EndpointGatingTests.cs diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 9368e27..18f8bc4 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 10 | +| Open findings | 6 | ## Summary @@ -87,10 +87,10 @@ via `GetOrAdd` so concurrent first-callers share one handler. Regression tests | | | |--|--| -| Severity | Medium | +| Severity | Medium — re-triaged: already fixed by the InboundAPI-001 fix; verified and closed | | Category | Concurrency & thread safety | -| Status | Open | -| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:123-129` | +| Status | Resolved | +| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:152-161` | **Description** @@ -111,7 +111,17 @@ return the handler it produced rather than requiring a separate dictionary read. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): re-triage — verified against the current +source, this finding was **already fixed** by the InboundAPI-001 fix. The +`InboundScriptExecutor.cs:152-161` lazy-compile path no longer does check-then-act +re-read: `Compile(method)` runs unconditionally (it never reads the cache) and the +result is published via the atomic `_scriptHandlers.GetOrAdd(method.Name, compiled)`. +There is no separate dictionary indexer read, so the `KeyNotFoundException` race the +finding describes cannot occur, and concurrent first-callers all share the single +handler that `GetOrAdd` keeps. Regression test +`LazyCompile_RacingRemoveHandler_NeverThrowsKeyNotFound` added (asserts a concurrent +`RemoveHandler` storm against lazy-compiling callers never yields the catch-all +"Internal script error"); it passes against the current code, confirming the fix. ### InboundAPI-003 — API key compared with non-constant-time string equality @@ -161,7 +171,7 @@ longer depends on it. |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:117-141` | **Description** @@ -185,7 +195,17 @@ or use a dedicated timeout `CancellationTokenSource` so the two are separable. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): `ExecuteAsync` now uses a dedicated timeout +`CancellationTokenSource` (`new CancellationTokenSource(timeout)`) linked with the +request-abort token, so the two cancellation sources are separable. The +`OperationCanceledException` handler reports "Script execution timed out" (and logs a +warning) **only** when the timeout CTS fired and the request token did not; a client +abort instead returns "Request cancelled by client" and logs at Debug — the failure +log stays reserved for genuine script-execution timeouts. `HandleInboundApiRequest` +additionally short-circuits with `Results.Empty` (no warning log, no 500 body write) +when `RequestAborted` is cancelled, since the connection is already gone. Regression +tests `ClientDisconnect_IsNotReportedAsTimeout` and `GenuineTimeout_StillReportedAsTimeout` +added. ### InboundAPI-005 — Compiled API scripts run with no script-trust-model enforcement @@ -237,7 +257,7 @@ Regression tests `CompileAndRegister_ForbiddenApi_RejectsScript` (theory), |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.InboundAPI/EndpointExtensions.cs:54-62` | **Description** @@ -260,16 +280,24 @@ Reject oversized bodies with 413 before buffering. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): added `InboundApiEndpointFilter`, an +`IEndpointFilter` applied to `POST /api/{methodName}` via `.AddEndpointFilter<>()`. +It rejects requests whose declared `Content-Length` exceeds `InboundApiOptions. +MaxRequestBodyBytes` (default 1 MiB) with HTTP 413 *before* the handler buffers the +body into a `JsonDocument`, and also lowers the per-request `IHttpMaxRequestBodySizeFeature` +cap so a chunked/unknown-length stream is cut off by Kestrel while being read. The +limit is configurable via the bound `ScadaLink:InboundApi` options section. Regression +tests `OversizedBody_ShortCircuitsWith413_AndDoesNotRunHandler`, `BodyAtLimit_RunsHandler`, +and `FilterCapsMaxRequestBodySizeFeature` added. ### InboundAPI-007 — `Database.Connection()` script API from the design doc is not implemented | | | |--|--| -| Severity | Medium | +| Severity | Medium — verified real drift; left Open pending a design decision (see Resolution) | | Category | Design-document adherence | | Status | Open | -| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:155-170` | +| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:188-203` | **Description** @@ -289,7 +317,27 @@ API. **Resolution** -_Unresolved._ +_Unresolved — left Open; needs a design decision the resolving agent cannot make._ +Re-triage 2026-05-16: confirmed against the current source — the drift is **real**. +`InboundScriptContext` (`InboundScriptExecutor.cs:188-203`) exposes only `Parameters`, +`Route`, and `CancellationToken`; there is no `Database` member, so a method script +following the documented `Database.Connection("name")` API fails to compile. + +This finding cannot be closed by the InboundAPI module agent for two reasons: +1. **Scope** — the alternative resolution (deleting the "Database Access" section) + edits `docs/requirements/Component-InboundAPI.md`, which is outside the editable + scope (`src/ScadaLink.InboundAPI`, `tests/`, this file only). +2. **It is a genuine design decision.** Implementing `Database.Connection()` is not a + mechanical fix: it hands inbound API scripts a *raw* MS SQL client. The ScadaLink + script trust model (CLAUDE.md, Akka.NET conventions) forbids scripts from `System.IO` + and raw network access, and `ForbiddenApiChecker` (added for InboundAPI-005) now + statically blocks `System.Net`/`System.IO`. A raw `SqlConnection` is in clear + tension with that trust model, and the set of connection names a script may open, + read-only vs. read-write access, and connection lifetime/pooling all require a + design call. **Surface to the design owner:** decide whether `Database.Connection()` + is in scope — if yes, write a design note covering the trust-model carve-out and + then implement a `Database` member backed by a connection-factory service; if no, + delete the "Database Access" section from `Component-InboundAPI.md`. ### InboundAPI-008 — Inbound API endpoint not restricted to the active central node @@ -297,7 +345,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.InboundAPI/EndpointExtensions.cs:19-23`, `src/ScadaLink.Host/Program.cs:149` | **Description** @@ -318,7 +366,19 @@ treated. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): introduced `IActiveNodeGate`, an abstraction +the inbound API uses to ask whether this node is the active (cluster-leader) central +node. The new `InboundApiEndpointFilter` (applied to `POST /api/{methodName}`) +consults the gate and short-circuits a standby node with HTTP 503 before any +auth/script work, so Traefik/clients only reach the live node — consistent with +`/health/active`. The gate is resolved optionally: when no implementation is +registered (non-clustered host / tests) the endpoint defaults to "allow", preserving +prior behaviour. Regression tests `StandbyNode_ShortCircuitsWith503_AndDoesNotRunHandler`, +`ActiveNode_PassesGate_RunsHandler`, and `NoGateRegistered_PassesGate_RunsHandler` +added. **Follow-up (outside this module's scope):** `ScadaLink.Host` should register +an `IActiveNodeGate` implementation backed by `ActiveNodeHealthCheck` / +`Cluster.State.Leader` in the central-role branch of `Program.cs` so the gate is +actually enforced in production; until then the endpoint defaults to "allow". ### InboundAPI-009 — Failed compilation is retried on every subsequent request diff --git a/src/ScadaLink.InboundAPI/EndpointExtensions.cs b/src/ScadaLink.InboundAPI/EndpointExtensions.cs index 7221a59..1749974 100644 --- a/src/ScadaLink.InboundAPI/EndpointExtensions.cs +++ b/src/ScadaLink.InboundAPI/EndpointExtensions.cs @@ -18,7 +18,10 @@ public static class EndpointExtensions { public static IEndpointRouteBuilder MapInboundAPI(this IEndpointRouteBuilder endpoints) { - endpoints.MapPost("/api/{methodName}", HandleInboundApiRequest); + endpoints.MapPost("/api/{methodName}", HandleInboundApiRequest) + // InboundAPI-006 / InboundAPI-008: active-node gating + request body + // size cap are enforced by the endpoint filter before the handler runs. + .AddEndpointFilter(); return endpoints; } @@ -86,6 +89,14 @@ public static class EndpointExtensions if (!scriptResult.Success) { + // InboundAPI-004: a client-aborted request is not a script failure. + // Do not pollute the failure log (reserved for genuine script errors) + // and do not attempt to write a 500 body to an already-gone connection. + if (httpContext.RequestAborted.IsCancellationRequested) + { + return Results.Empty; + } + // WP-5: 500 for script failures, safe error message logger.LogWarning( "Inbound API script failure for method {Method}: {Error}", diff --git a/src/ScadaLink.InboundAPI/IActiveNodeGate.cs b/src/ScadaLink.InboundAPI/IActiveNodeGate.cs new file mode 100644 index 0000000..0fd01d4 --- /dev/null +++ b/src/ScadaLink.InboundAPI/IActiveNodeGate.cs @@ -0,0 +1,24 @@ +namespace ScadaLink.InboundAPI; + +/// +/// InboundAPI-008: abstraction the inbound API endpoint uses to determine whether +/// this node is the active (cluster-leader) central node. +/// +/// The design states the inbound API is "Central cluster only (active node)" and +/// "fails over with it". A standby central node must not execute method scripts or +/// Route.To() calls — that can race the active node or run against stale +/// singleton state. consults this gate and +/// returns HTTP 503 from a standby so Traefik/clients only reach the live node. +/// +/// The implementation lives in the Host (it needs Akka cluster state); when no +/// implementation is registered, the endpoint defaults to "allow" so non-clustered +/// hosts and tests are unaffected. +/// +public interface IActiveNodeGate +{ + /// + /// true when this node is the active central node and may serve the + /// inbound API; false on a standby node. + /// + bool IsActiveNode { get; } +} diff --git a/src/ScadaLink.InboundAPI/InboundApiEndpointFilter.cs b/src/ScadaLink.InboundAPI/InboundApiEndpointFilter.cs new file mode 100644 index 0000000..747f37b --- /dev/null +++ b/src/ScadaLink.InboundAPI/InboundApiEndpointFilter.cs @@ -0,0 +1,78 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; + +namespace ScadaLink.InboundAPI; + +/// +/// Endpoint filter applied to POST /api/{methodName} that enforces two +/// cross-cutting guards before the request handler runs: +/// +/// +/// +/// InboundAPI-008 — active-node gating. The inbound API is central-active-node-only; +/// a standby node returns HTTP 503 so it never executes method scripts. +/// +/// +/// InboundAPI-006 — request body size cap. Oversized bodies are rejected with HTTP +/// 413 before being buffered into a JsonDocument. +/// +/// +/// +public sealed class InboundApiEndpointFilter : IEndpointFilter +{ + private readonly ILogger _logger; + private readonly InboundApiOptions _options; + + public InboundApiEndpointFilter( + ILogger logger, + IOptions options) + { + _logger = logger; + _options = options.Value; + } + + public async ValueTask InvokeAsync( + EndpointFilterInvocationContext context, + EndpointFilterDelegate next) + { + var httpContext = context.HttpContext; + + // InboundAPI-008: refuse to serve the inbound API on a standby central node. + // The gate is optional — when no IActiveNodeGate is registered (non-clustered + // host / tests) the API is served, preserving prior behaviour. + var gate = httpContext.RequestServices.GetService(); + if (gate is { IsActiveNode: false }) + { + _logger.LogWarning( + "Inbound API request rejected — this node is a standby (not the active central node)"); + return Results.Json( + new { error = "Inbound API is only available on the active central node" }, + statusCode: StatusCodes.Status503ServiceUnavailable); + } + + // InboundAPI-006: cap the request body size. Reject an over-limit body up + // front via Content-Length; also lower the per-request max body size so a + // chunked/unknown-length stream is cut off by Kestrel as it is read. + var maxBytes = _options.MaxRequestBodyBytes; + if (httpContext.Request.ContentLength is { } declaredLength && declaredLength > maxBytes) + { + _logger.LogWarning( + "Inbound API request rejected — body length {Length} exceeds limit {Limit}", + declaredLength, maxBytes); + return Results.Json( + new { error = "Request body too large" }, + statusCode: StatusCodes.Status413PayloadTooLarge); + } + + var sizeFeature = httpContext.Features.Get(); + if (sizeFeature is { IsReadOnly: false }) + { + sizeFeature.MaxRequestBodySize = maxBytes; + } + + return await next(context); + } +} diff --git a/src/ScadaLink.InboundAPI/InboundApiOptions.cs b/src/ScadaLink.InboundAPI/InboundApiOptions.cs index bb2b65e..f25a6f3 100644 --- a/src/ScadaLink.InboundAPI/InboundApiOptions.cs +++ b/src/ScadaLink.InboundAPI/InboundApiOptions.cs @@ -2,5 +2,19 @@ namespace ScadaLink.InboundAPI; public class InboundApiOptions { + /// + /// Default cap on the inbound API request body, in bytes (InboundAPI-006). + /// + public const long DefaultMaxRequestBodyBytes = 1L * 1024 * 1024; // 1 MiB + public TimeSpan DefaultMethodTimeout { get; set; } = TimeSpan.FromSeconds(30); + + /// + /// InboundAPI-006: maximum accepted request body size for POST /api/{methodName}. + /// Requests whose body exceeds this are rejected with HTTP 413 before being + /// buffered into a . The inbound API + /// has no rate limiting (a deliberate design choice), so an explicit, modest cap + /// bounds per-request allocations. + /// + public long MaxRequestBodyBytes { get; set; } = DefaultMaxRequestBodyBytes; } diff --git a/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs b/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs index ddbe526..094c482 100644 --- a/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs +++ b/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs @@ -142,11 +142,17 @@ public class InboundScriptExecutor TimeSpan timeout, CancellationToken cancellationToken = default) { + // InboundAPI-004: keep the timeout source and the request-abort source + // separable. A single linked CTS makes a genuine client disconnect + // indistinguishable from a method timeout, so a normal disconnect would be + // logged and reported as "Script execution timed out". Use a dedicated + // timeout CTS, linked with the request token, so the two can be told apart. + using var timeoutCts = new CancellationTokenSource(timeout); + using var cts = CancellationTokenSource.CreateLinkedTokenSource( + cancellationToken, timeoutCts.Token); + try { - using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - cts.CancelAfter(timeout); - var context = new InboundScriptContext(parameters, route, cts.Token); if (!_scriptHandlers.TryGetValue(method.Name, out var handler)) @@ -170,8 +176,18 @@ public class InboundScriptExecutor } catch (OperationCanceledException) { - _logger.LogWarning("Script execution timed out for method {Method}", method.Name); - return new InboundScriptResult(false, null, "Script execution timed out"); + // InboundAPI-004: distinguish a genuine method timeout from a client + // abort. Only the timeout CTS firing is a real timeout; if the caller's + // request token fired, the client disconnected — do not pollute the + // timeout log (reserved for genuine script-execution timeouts). + if (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + _logger.LogWarning("Script execution timed out for method {Method}", method.Name); + return new InboundScriptResult(false, null, "Script execution timed out"); + } + + _logger.LogDebug("Inbound API request for method {Method} cancelled by client", method.Name); + return new InboundScriptResult(false, null, "Request cancelled by client"); } catch (Exception ex) { diff --git a/src/ScadaLink.InboundAPI/ServiceCollectionExtensions.cs b/src/ScadaLink.InboundAPI/ServiceCollectionExtensions.cs index 1dcc91f..d67d454 100644 --- a/src/ScadaLink.InboundAPI/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.InboundAPI/ServiceCollectionExtensions.cs @@ -10,6 +10,10 @@ public static class ServiceCollectionExtensions services.AddSingleton(); services.AddScoped(); + // InboundAPI-006 / InboundAPI-008: endpoint filter enforcing the request + // body size cap and active-node gating for POST /api/{methodName}. + services.AddSingleton(); + return services; } } diff --git a/tests/ScadaLink.InboundAPI.Tests/EndpointGatingTests.cs b/tests/ScadaLink.InboundAPI.Tests/EndpointGatingTests.cs new file mode 100644 index 0000000..d27dc1d --- /dev/null +++ b/tests/ScadaLink.InboundAPI.Tests/EndpointGatingTests.cs @@ -0,0 +1,147 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; + +namespace ScadaLink.InboundAPI.Tests; + +/// +/// InboundAPI-006 / InboundAPI-008: the POST /api/{methodName} endpoint must be +/// gated to the active central node and must cap the request body size. These +/// behaviours are enforced by . +/// +public class EndpointGatingTests +{ + private static InboundApiEndpointFilter CreateFilter(InboundApiOptions? options = null) => + new(NullLogger.Instance, + Options.Create(options ?? new InboundApiOptions())); + + private static (DefaultHttpContext ctx, DefaultEndpointFilterInvocationContext invocation) + BuildInvocation(bool? activeNode, long? contentLength) + { + var services = new ServiceCollection(); + if (activeNode.HasValue) + services.AddSingleton(new StubActiveNodeGate(activeNode.Value)); + + var ctx = new DefaultHttpContext + { + RequestServices = services.BuildServiceProvider() + }; + ctx.Request.Method = "POST"; + if (contentLength.HasValue) + ctx.Request.ContentLength = contentLength.Value; + + return (ctx, new DefaultEndpointFilterInvocationContext(ctx)); + } + + private static EndpointFilterDelegate NextSentinel(out Func wasCalled) + { + var called = false; + wasCalled = () => called; + return _ => + { + called = true; + return ValueTask.FromResult(Results.Ok("handler-ran")); + }; + } + + // --- InboundAPI-008: standby node must not serve inbound API calls --- + + [Fact] + public async Task StandbyNode_ShortCircuitsWith503_AndDoesNotRunHandler() + { + var (_, invocation) = BuildInvocation(activeNode: false, contentLength: 2); + var next = NextSentinel(out var handlerRan); + + var result = await CreateFilter().InvokeAsync(invocation, next); + + Assert.False(handlerRan()); + var status = Assert.IsAssignableFrom(result); + Assert.Equal(StatusCodes.Status503ServiceUnavailable, status.StatusCode); + } + + [Fact] + public async Task ActiveNode_PassesGate_RunsHandler() + { + var (_, invocation) = BuildInvocation(activeNode: true, contentLength: 2); + var next = NextSentinel(out var handlerRan); + + await CreateFilter().InvokeAsync(invocation, next); + + Assert.True(handlerRan()); + } + + [Fact] + public async Task NoGateRegistered_PassesGate_RunsHandler() + { + // When no IActiveNodeGate is registered (non-clustered host), gating is + // opt-in and defaults to "allow" so the endpoint is still served. + var (_, invocation) = BuildInvocation(activeNode: null, contentLength: 2); + var next = NextSentinel(out var handlerRan); + + await CreateFilter().InvokeAsync(invocation, next); + + Assert.True(handlerRan()); + } + + // --- InboundAPI-006: request body size must be capped --- + + [Fact] + public async Task OversizedBody_ShortCircuitsWith413_AndDoesNotRunHandler() + { + var options = new InboundApiOptions(); + var (_, invocation) = BuildInvocation( + activeNode: true, contentLength: options.MaxRequestBodyBytes + 1); + var next = NextSentinel(out var handlerRan); + + var result = await CreateFilter(options).InvokeAsync(invocation, next); + + Assert.False(handlerRan()); + var status = Assert.IsAssignableFrom(result); + Assert.Equal(StatusCodes.Status413PayloadTooLarge, status.StatusCode); + } + + [Fact] + public async Task BodyAtLimit_RunsHandler() + { + var options = new InboundApiOptions(); + var (_, invocation) = BuildInvocation( + activeNode: true, contentLength: options.MaxRequestBodyBytes); + var next = NextSentinel(out var handlerRan); + + await CreateFilter(options).InvokeAsync(invocation, next); + + Assert.True(handlerRan()); + } + + [Fact] + public async Task FilterCapsMaxRequestBodySizeFeature() + { + // For chunked/unknown-length requests there is no Content-Length, so the + // filter must also cap the per-request body size feature so Kestrel rejects + // an oversized stream while it is being read. + var options = new InboundApiOptions(); + var (ctx, invocation) = BuildInvocation(activeNode: true, contentLength: null); + ctx.Features.Set(new StubMaxBodySizeFeature()); + var next = NextSentinel(out _); + + await CreateFilter(options).InvokeAsync(invocation, next); + + var feature = ctx.Features.Get(); + Assert.Equal(options.MaxRequestBodyBytes, feature!.MaxRequestBodySize); + } + + private sealed class StubActiveNodeGate : IActiveNodeGate + { + private readonly bool _isActive; + public StubActiveNodeGate(bool isActive) => _isActive = isActive; + public bool IsActiveNode => _isActive; + } + + private sealed class StubMaxBodySizeFeature : IHttpMaxRequestBodySizeFeature + { + public bool IsReadOnly => false; + public long? MaxRequestBodySize { get; set; } + } +} diff --git a/tests/ScadaLink.InboundAPI.Tests/InboundScriptExecutorTests.cs b/tests/ScadaLink.InboundAPI.Tests/InboundScriptExecutorTests.cs index 92c8c64..e564ae5 100644 --- a/tests/ScadaLink.InboundAPI.Tests/InboundScriptExecutorTests.cs +++ b/tests/ScadaLink.InboundAPI.Tests/InboundScriptExecutorTests.cs @@ -239,4 +239,88 @@ public class InboundScriptExecutorTests Assert.True(_executor.CompileAndRegister(method)); } + + // --- InboundAPI-002: lazy compile-and-fetch must be atomic, never KeyNotFoundException --- + + [Fact] + public async Task LazyCompile_RacingRemoveHandler_NeverThrowsKeyNotFound() + { + // The lazy-compile path must compile-and-fetch atomically: a concurrent + // RemoveHandler must not be able to turn a first-call into an "Internal + // script error" (the old check-then-act re-read could throw KeyNotFoundException). + var method = new ApiMethod("atomic", "return 5;") { Id = 1, TimeoutSeconds = 10 }; + + var removers = Enumerable.Range(0, 16).Select(_ => Task.Run(() => + { + for (var n = 0; n < 200; n++) + _executor.RemoveHandler("atomic"); + })); + + var callers = Enumerable.Range(0, 16).Select(_ => Task.Run(async () => + { + for (var n = 0; n < 50; n++) + { + var r = await _executor.ExecuteAsync( + method, new Dictionary(), _route, TimeSpan.FromSeconds(10)); + // Result must always be a clean success or a clean compilation + // failure — never the catch-all "Internal script error". + Assert.NotEqual("Internal script error", r.ErrorMessage); + } + })); + + await Task.WhenAll(removers.Concat(callers)); + } + + // --- InboundAPI-004: a client disconnect must NOT be reported as a script timeout --- + + [Fact] + public async Task ClientDisconnect_IsNotReportedAsTimeout() + { + // When the caller's request token is cancelled (client aborted the request), + // ExecuteAsync must report a client-cancelled failure, not "Script execution + // timed out" — that log line is reserved for genuine timeouts. + var method = new ApiMethod("aborted", "return 1;") { Id = 1, TimeoutSeconds = 30 }; + _executor.RegisterHandler("aborted", async ctx => + { + await Task.Delay(TimeSpan.FromSeconds(60), ctx.CancellationToken); + return "never"; + }); + + using var clientAborted = new CancellationTokenSource(); + clientAborted.CancelAfter(TimeSpan.FromMilliseconds(100)); + + var result = await _executor.ExecuteAsync( + method, + new Dictionary(), + _route, + // Generous method timeout so the timeout CTS is NOT the cause. + TimeSpan.FromSeconds(30), + clientAborted.Token); + + Assert.False(result.Success); + Assert.DoesNotContain("timed out", result.ErrorMessage ?? string.Empty); + } + + [Fact] + public async Task GenuineTimeout_StillReportedAsTimeout() + { + // A method that exceeds its timeout with no client abort must still be + // reported as "timed out" (regression guard for the InboundAPI-004 fix). + var method = new ApiMethod("genuine", "return 1;") { Id = 1, TimeoutSeconds = 1 }; + _executor.RegisterHandler("genuine", async ctx => + { + await Task.Delay(TimeSpan.FromSeconds(60), ctx.CancellationToken); + return "never"; + }); + + var result = await _executor.ExecuteAsync( + method, + new Dictionary(), + _route, + TimeSpan.FromMilliseconds(100), + CancellationToken.None); + + Assert.False(result.Success); + Assert.Contains("timed out", result.ErrorMessage); + } } diff --git a/tests/ScadaLink.InboundAPI.Tests/ScadaLink.InboundAPI.Tests.csproj b/tests/ScadaLink.InboundAPI.Tests/ScadaLink.InboundAPI.Tests.csproj index 0ab5147..a29815f 100644 --- a/tests/ScadaLink.InboundAPI.Tests/ScadaLink.InboundAPI.Tests.csproj +++ b/tests/ScadaLink.InboundAPI.Tests/ScadaLink.InboundAPI.Tests.csproj @@ -8,6 +8,10 @@ false + + + +