diff --git a/src/ScadaLink.InboundAPI/Middleware/AuditWriteMiddleware.cs b/src/ScadaLink.InboundAPI/Middleware/AuditWriteMiddleware.cs index 2662f3f..c271d03 100644 --- a/src/ScadaLink.InboundAPI/Middleware/AuditWriteMiddleware.cs +++ b/src/ScadaLink.InboundAPI/Middleware/AuditWriteMiddleware.cs @@ -148,6 +148,13 @@ public sealed class AuditWriteMiddleware // Audit Log #23: a fresh per-request correlation id so the // inbound row carries a request identifier (closes the design // gap that inbound rows should be correlatable). + // + // This id is intentionally request-local: it is NOT bridged to + // RouteHelper's routed-call correlation id or to + // HttpContext.TraceIdentifier. Threading an inbound request's + // correlation id through to the routed script execution (so an + // inbound call and the outbound API/DB rows it triggers share + // one id) is a deliberate future follow-up, out of scope here. CorrelationId = Guid.NewGuid(), Actor = actor, Target = methodName, diff --git a/src/ScadaLink.SiteRuntime/Scripts/AuditingDbCommand.cs b/src/ScadaLink.SiteRuntime/Scripts/AuditingDbCommand.cs index a056214..3e59ecf 100644 --- a/src/ScadaLink.SiteRuntime/Scripts/AuditingDbCommand.cs +++ b/src/ScadaLink.SiteRuntime/Scripts/AuditingDbCommand.cs @@ -37,10 +37,13 @@ internal sealed class AuditingDbCommand : DbCommand private readonly string _siteId; private readonly string _instanceName; private readonly string? _sourceScript; - private readonly Guid _correlationId; + private readonly Guid _auditCorrelationId; private readonly ILogger _logger; private DbConnection? _wrappingConnection; + // Parameter ordering: auditCorrelationId sits immediately after the ILogger, + // consistent with the other three audit-threaded ctors (ExternalSystemHelper, + // DatabaseHelper, AuditingDbConnection). public AuditingDbCommand( DbCommand inner, IAuditWriter auditWriter, @@ -48,8 +51,8 @@ internal sealed class AuditingDbCommand : DbCommand string siteId, string instanceName, string? sourceScript, - Guid correlationId, - ILogger logger) + ILogger logger, + Guid auditCorrelationId) { _inner = inner ?? throw new ArgumentNullException(nameof(inner)); _auditWriter = auditWriter ?? throw new ArgumentNullException(nameof(auditWriter)); @@ -57,8 +60,8 @@ internal sealed class AuditingDbCommand : DbCommand _siteId = siteId ?? string.Empty; _instanceName = instanceName ?? string.Empty; _sourceScript = sourceScript; - _correlationId = correlationId; _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + _auditCorrelationId = auditCorrelationId; } // -- Forwarded surface ------------------------------------------------ @@ -429,9 +432,10 @@ internal sealed class AuditingDbCommand : DbCommand OccurredAtUtc = DateTime.SpecifyKind(occurredAtUtc, DateTimeKind.Utc), Channel = AuditChannel.DbOutbound, Kind = AuditKind.DbWrite, - // Audit Log #23: the execution-wide correlation id, so all the - // sync ApiCall/DbWrite rows from one script run share an id. - CorrelationId = _correlationId, + // Audit Log #23: the execution-wide correlation id, so this sync + // DbWrite row shares an id with the other sync trust-boundary rows + // from the same script run. + CorrelationId = _auditCorrelationId, SourceSiteId = string.IsNullOrEmpty(_siteId) ? null : _siteId, SourceInstanceId = _instanceName, SourceScript = _sourceScript, diff --git a/src/ScadaLink.SiteRuntime/Scripts/AuditingDbConnection.cs b/src/ScadaLink.SiteRuntime/Scripts/AuditingDbConnection.cs index 33edf62..a8d0ee0 100644 --- a/src/ScadaLink.SiteRuntime/Scripts/AuditingDbConnection.cs +++ b/src/ScadaLink.SiteRuntime/Scripts/AuditingDbConnection.cs @@ -36,9 +36,12 @@ internal sealed class AuditingDbConnection : DbConnection private readonly string _siteId; private readonly string _instanceName; private readonly string? _sourceScript; - private readonly Guid _correlationId; + private readonly Guid _auditCorrelationId; private readonly ILogger _logger; + // Parameter ordering: auditCorrelationId sits immediately after the ILogger, + // consistent with the other three audit-threaded ctors (ExternalSystemHelper, + // DatabaseHelper, AuditingDbCommand). public AuditingDbConnection( DbConnection inner, IAuditWriter auditWriter, @@ -46,8 +49,8 @@ internal sealed class AuditingDbConnection : DbConnection string siteId, string instanceName, string? sourceScript, - Guid correlationId, - ILogger logger) + ILogger logger, + Guid auditCorrelationId) { _inner = inner ?? throw new ArgumentNullException(nameof(inner)); _auditWriter = auditWriter ?? throw new ArgumentNullException(nameof(auditWriter)); @@ -55,8 +58,8 @@ internal sealed class AuditingDbConnection : DbConnection _siteId = siteId ?? string.Empty; _instanceName = instanceName ?? string.Empty; _sourceScript = sourceScript; - _correlationId = correlationId; _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + _auditCorrelationId = auditCorrelationId; } // ConnectionString is settable on DbConnection — forward both halves. @@ -95,8 +98,8 @@ internal sealed class AuditingDbConnection : DbConnection _siteId, _instanceName, _sourceScript, - _correlationId, - _logger); + _logger, + _auditCorrelationId); } protected override void Dispose(bool disposing) diff --git a/src/ScadaLink.SiteRuntime/Scripts/ScriptRuntimeContext.cs b/src/ScadaLink.SiteRuntime/Scripts/ScriptRuntimeContext.cs index 1708173..ed643ef 100644 --- a/src/ScadaLink.SiteRuntime/Scripts/ScriptRuntimeContext.cs +++ b/src/ScadaLink.SiteRuntime/Scripts/ScriptRuntimeContext.cs @@ -111,9 +111,9 @@ public class ScriptRuntimeContext /// (ApiCall, DbWrite) is stamped with this id so all the /// rows from one script run can be correlated together. /// - private readonly Guid _correlationId; + private readonly Guid _auditCorrelationId; - /// + /// /// Audit Log #23: the execution-wide audit correlation id. When omitted /// (tag-change / timer-triggered executions) a fresh id is generated; an /// inbound caller may supply one to tie the execution to an upstream @@ -138,7 +138,7 @@ public class ScriptRuntimeContext IAuditWriter? auditWriter = null, IOperationTrackingStore? operationTrackingStore = null, ICachedCallTelemetryForwarder? cachedForwarder = null, - Guid? correlationId = null) + Guid? auditCorrelationId = null) { _instanceActor = instanceActor; _self = self; @@ -157,7 +157,7 @@ public class ScriptRuntimeContext _auditWriter = auditWriter; _operationTrackingStore = operationTrackingStore; _cachedForwarder = cachedForwarder; - _correlationId = correlationId ?? Guid.NewGuid(); + _auditCorrelationId = auditCorrelationId ?? Guid.NewGuid(); } /// @@ -258,7 +258,7 @@ public class ScriptRuntimeContext /// ExternalSystem.CachedCall("systemName", "methodName", params) /// public ExternalSystemHelper ExternalSystem => new( - _externalSystemClient, _instanceName, _logger, _correlationId, _auditWriter, _siteId, _sourceScript, + _externalSystemClient, _instanceName, _logger, _auditCorrelationId, _auditWriter, _siteId, _sourceScript, // Audit Log #23 (M3 Bundle E — Task E3): emit CachedSubmit telemetry // on every ExternalSystem.CachedCall enqueue. _cachedForwarder); @@ -272,7 +272,7 @@ public class ScriptRuntimeContext _databaseGateway, _instanceName, _logger, - _correlationId, + _auditCorrelationId, // Audit Log #23 (M4 Bundle A): wire the IAuditWriter so // Database.Connection(name) returns an auditing decorator that // emits one DbOutbound/DbWrite row per script-initiated @@ -380,7 +380,7 @@ public class ScriptRuntimeContext private readonly IExternalSystemClient? _client; private readonly string _instanceName; private readonly ILogger _logger; - private readonly Guid _correlationId; + private readonly Guid _auditCorrelationId; private readonly IAuditWriter? _auditWriter; private readonly string _siteId; private readonly string? _sourceScript; @@ -389,11 +389,18 @@ public class ScriptRuntimeContext // Internal constructor for tests living in ScadaLink.SiteRuntime.Tests // (via InternalsVisibleTo). Production sites resolve the helper through // ScriptRuntimeContext.ExternalSystem. + // + // Parameter ordering: auditCorrelationId sits immediately after the + // ILogger across all four audit-threaded ctors (ExternalSystemHelper, + // DatabaseHelper, AuditingDbConnection, AuditingDbCommand) — a required + // Guid cannot follow the optional provenance params without a + // required-after-optional compile error, so the post-logger slot is the + // one consistent position that compiles cleanly everywhere. internal ExternalSystemHelper( IExternalSystemClient? client, string instanceName, ILogger logger, - Guid correlationId, + Guid auditCorrelationId, IAuditWriter? auditWriter = null, string siteId = "", string? sourceScript = null, @@ -402,7 +409,7 @@ public class ScriptRuntimeContext _client = client; _instanceName = instanceName; _logger = logger; - _correlationId = correlationId; + _auditCorrelationId = auditCorrelationId; _auditWriter = auditWriter; _siteId = siteId; _sourceScript = sourceScript; @@ -905,7 +912,7 @@ public class ScriptRuntimeContext Kind = AuditKind.ApiCall, // Audit Log #23: the execution-wide correlation id, so all the // sync ApiCall/DbWrite rows from one script run share an id. - CorrelationId = _correlationId, + CorrelationId = _auditCorrelationId, SourceSiteId = string.IsNullOrEmpty(_siteId) ? null : _siteId, SourceInstanceId = _instanceName, SourceScript = _sourceScript, @@ -972,7 +979,7 @@ public class ScriptRuntimeContext private readonly IDatabaseGateway? _gateway; private readonly string _instanceName; private readonly ILogger _logger; - private readonly Guid _correlationId; + private readonly Guid _auditCorrelationId; private readonly string _siteId; private readonly string? _sourceScript; private readonly ICachedCallTelemetryForwarder? _cachedForwarder; @@ -989,11 +996,15 @@ public class ScriptRuntimeContext /// private readonly IAuditWriter? _auditWriter; + // Parameter ordering: auditCorrelationId sits immediately after the + // ILogger — see the note on ExternalSystemHelper's ctor for why the + // post-logger slot is the one consistent position across all four + // audit-threaded ctors. internal DatabaseHelper( IDatabaseGateway? gateway, string instanceName, ILogger logger, - Guid correlationId, + Guid auditCorrelationId, IAuditWriter? auditWriter = null, string siteId = "", string? sourceScript = null, @@ -1002,7 +1013,7 @@ public class ScriptRuntimeContext _gateway = gateway; _instanceName = instanceName; _logger = logger; - _correlationId = correlationId; + _auditCorrelationId = auditCorrelationId; _auditWriter = auditWriter; _siteId = siteId; _sourceScript = sourceScript; @@ -1037,8 +1048,8 @@ public class ScriptRuntimeContext siteId: _siteId, instanceName: _instanceName, sourceScript: _sourceScript, - correlationId: _correlationId, - logger: _logger); + logger: _logger, + auditCorrelationId: _auditCorrelationId); } /// diff --git a/tests/ScadaLink.SiteRuntime.Tests/Scripts/ExecutionCorrelationContextTests.cs b/tests/ScadaLink.SiteRuntime.Tests/Scripts/ExecutionCorrelationContextTests.cs new file mode 100644 index 0000000..13a54dc --- /dev/null +++ b/tests/ScadaLink.SiteRuntime.Tests/Scripts/ExecutionCorrelationContextTests.cs @@ -0,0 +1,179 @@ +using Akka.Actor; +using Microsoft.Data.Sqlite; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using ScadaLink.Commons.Entities.Audit; +using ScadaLink.Commons.Interfaces.Services; +using ScadaLink.Commons.Types.Enums; +using ScadaLink.SiteRuntime.Scripts; + +namespace ScadaLink.SiteRuntime.Tests.Scripts; + +/// +/// Audit Log #23 — execution-correlation tests exercised through a full +/// : +/// +/// +/// +/// The ?? Guid.NewGuid() fallback in the +/// ctor: when no audit correlation id is supplied (tag-change / timer-triggered +/// executions) a fresh, non-empty id is minted and stamped on the emitted rows. +/// +/// +/// The execution-wide contract: an ExternalSystem.Call and a sync +/// Database write performed through ONE context share a single +/// . +/// +/// +/// +public class ExecutionCorrelationContextTests +{ + /// + /// In-memory capturing every emitted event + /// (mirrors the CapturingAuditWriter stubs in + /// / + /// ). + /// + private sealed class CapturingAuditWriter : IAuditWriter + { + public List Events { get; } = new(); + + public Task WriteAsync(AuditEvent evt, CancellationToken ct = default) + { + Events.Add(evt); + return Task.CompletedTask; + } + } + + private const string InstanceName = "Plant.Pump42"; + private const string ConnectionName = "machineData"; + + /// + /// Builds a full wired with the external + /// system client, database gateway and audit writer the cross-helper test + /// needs. The actor refs are — the + /// integration helpers (ExternalSystem / Database) never touch them — and + /// defaults to null so the ctor's + /// ?? Guid.NewGuid() fallback is exercised unless a test supplies one. + /// + private static ScriptRuntimeContext CreateContext( + IExternalSystemClient? externalSystemClient, + IDatabaseGateway? databaseGateway, + IAuditWriter? auditWriter, + Guid? auditCorrelationId = null) + { + var compilationService = new ScriptCompilationService( + NullLogger.Instance); + var sharedScriptLibrary = new SharedScriptLibrary( + compilationService, NullLogger.Instance); + + return new ScriptRuntimeContext( + ActorRefs.Nobody, + ActorRefs.Nobody, + sharedScriptLibrary, + currentCallDepth: 0, + maxCallDepth: 10, + askTimeout: TimeSpan.FromSeconds(5), + instanceName: InstanceName, + logger: NullLogger.Instance, + externalSystemClient: externalSystemClient, + databaseGateway: databaseGateway, + storeAndForward: null, + siteCommunicationActor: null, + siteId: "site-77", + sourceScript: "ScriptActor:OnTick", + auditWriter: auditWriter, + operationTrackingStore: null, + cachedForwarder: null, + auditCorrelationId: auditCorrelationId); + } + + /// + /// Spin up a fresh in-memory SQLite database with a tiny single-table + /// schema. The keep-alive root must outlive any auditing wrapper the test + /// exercises (mirrors DatabaseSyncEmissionTests.NewInMemoryDb). + /// + private static SqliteConnection NewInMemoryDb(out SqliteConnection keepAlive) + { + var dbName = $"db-{Guid.NewGuid():N}"; + var connStr = $"Data Source={dbName};Mode=Memory;Cache=Shared"; + + keepAlive = new SqliteConnection(connStr); + keepAlive.Open(); + using (var seed = keepAlive.CreateCommand()) + { + seed.CommandText = + "CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT NOT NULL);"; + seed.ExecuteNonQuery(); + } + + var live = new SqliteConnection(connStr); + live.Open(); + return live; + } + + [Fact] + public async Task NoCorrelationIdSupplied_SyncCall_StampsFreshNonEmptyCorrelationId() + { + // No auditCorrelationId argument — the ScriptRuntimeContext ctor's + // `?? Guid.NewGuid()` fallback must mint one (this is the unsupplied-id + // branch every other audit test bypasses by passing an explicit id). + var client = new Mock(); + client + .Setup(c => c.CallAsync("ERP", "GetOrder", It.IsAny?>(), It.IsAny())) + .ReturnsAsync(new ExternalCallResult(true, "{}", null)); + var writer = new CapturingAuditWriter(); + + var context = CreateContext(client.Object, databaseGateway: null, writer); + await context.ExternalSystem.Call("ERP", "GetOrder"); + + var evt = Assert.Single(writer.Events); + Assert.NotNull(evt.CorrelationId); + Assert.NotEqual(Guid.Empty, evt.CorrelationId!.Value); + } + + [Fact] + public async Task SameContext_ApiCallAndDbWrite_ShareTheSameCorrelationId() + { + // The execution-wide contract: an ExternalSystem.Call AND a sync + // Database write performed through ONE ScriptRuntimeContext must both + // carry the same execution correlation id, so an audit reader can tie + // every trust-boundary action from one script run together. + using var keepAlive = new SqliteConnection("Data Source=ecc;Mode=Memory;Cache=Shared"); + var innerDb = NewInMemoryDb(out var _); + + var client = new Mock(); + client + .Setup(c => c.CallAsync("ERP", "GetOrder", It.IsAny?>(), It.IsAny())) + .ReturnsAsync(new ExternalCallResult(true, "{}", null)); + + var gateway = new Mock(); + gateway + .Setup(g => g.GetConnectionAsync(ConnectionName, It.IsAny())) + .ReturnsAsync(innerDb); + + var writer = new CapturingAuditWriter(); + var context = CreateContext(client.Object, gateway.Object, writer); + + // 1) outbound API call through the context's ExternalSystem helper. + await context.ExternalSystem.Call("ERP", "GetOrder"); + + // 2) sync DB write through the SAME context's Database helper. + await using (var conn = await context.Database.Connection(ConnectionName)) + await using (var cmd = conn.CreateCommand()) + { + cmd.CommandText = "INSERT INTO t (id, name) VALUES (1, 'alpha')"; + await cmd.ExecuteNonQueryAsync(); + } + + Assert.Equal(2, writer.Events.Count); + var apiRow = Assert.Single(writer.Events, e => e.Channel == AuditChannel.ApiOutbound); + var dbRow = Assert.Single(writer.Events, e => e.Channel == AuditChannel.DbOutbound); + + Assert.NotNull(apiRow.CorrelationId); + Assert.NotEqual(Guid.Empty, apiRow.CorrelationId!.Value); + // The ApiCall row and the DbWrite row, emitted by two different helpers + // resolved off one context, carry the identical execution correlation id. + Assert.Equal(apiRow.CorrelationId, dbRow.CorrelationId); + } +}