From e79452d2deeb3660a6e49d70ed137066bad2fc18 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 6 Sep 2026 14:15:38 -0400 Subject: [PATCH] fix(site-runtime): Flow Debugger inline trace copy is redacted and byte-capped at the collector (arch-review F1, site half) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every emit site hands FlowTraceCollector the same AuditEvent it hands the writer, but redaction and payload capping lived inside the writers, which return a NEW filtered record — so the inline copy that rides ScriptCallResult.TraceEvents / RouteToCallResponse.TraceEvents was the RAW event: no per-target SQL-parameter redaction, no header scrubbing, no 8 KB / 64 KB truncation. Live and Replay of the same execution could therefore show different payloads, and the 500-event-only cap left the reply free to breach the 4 MB gRPC receive limit — which would fail a real call, since the same reply carries the script's return value. The collector now applies the host IAuditRedactor itself (outside the lock; the writers keep their own idempotent Apply) and enforces a byte budget (DefaultMaxBytes = 1 MB over the redacted DetailsJson) alongside the event cap, flagging Truncated when either trips. It moves to Commons (Types/Debugging) so the central FlowDebugRunner can share it. The redactor is a singleton, so ScriptRunLauncher resolves it from the ROOT provider and keeps constructing the collector before the try — the traced ScriptRun row is emitted from the catch blocks too, including the shed-at-dequeue path that throws before the DI scope exists. Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29 --- .../Types/Debugging}/FlowTraceCollector.cs | 52 +++++++++++++++--- .../Scripts/ScriptRunLauncher.cs | 10 +++- .../Debugging}/FlowTraceCollectorTests.cs | 54 ++++++++++++++++++- .../ScriptRuntimeContextTraceEmissionTests.cs | 33 ++++++++++++ 4 files changed, 138 insertions(+), 11 deletions(-) rename src/{ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts => ZB.MOM.WW.ScadaBridge.Commons/Types/Debugging}/FlowTraceCollector.cs (53%) rename tests/{ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts => ZB.MOM.WW.ScadaBridge.Commons.Tests/Debugging}/FlowTraceCollectorTests.cs (57%) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/FlowTraceCollector.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Debugging/FlowTraceCollector.cs similarity index 53% rename from src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/FlowTraceCollector.cs rename to src/ZB.MOM.WW.ScadaBridge.Commons/Types/Debugging/FlowTraceCollector.cs index 101b561e..aa92efc1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/FlowTraceCollector.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Debugging/FlowTraceCollector.cs @@ -1,6 +1,6 @@ using ZB.MOM.WW.Audit; -namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; +namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Debugging; /// /// Per-run sink for ScriptTrace audit events so a traced routed call can return them @@ -14,17 +14,37 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; /// when the launcher takes the reply snapshot — a lifecycle row the S&F forwarder writes /// minutes later, or an un-awaited call finishing after the run returned, must not mutate a /// list the reply has already been built from. is a no-op once completed. +/// Arch-review F1 (2026-09-06): the inline copy is REDACTED — the collector applies the +/// host's itself rather than storing the raw event the emitter also +/// hands the writer, so Live shows exactly what Replay reads back (per-target SQL-parameter +/// overrides, header scrubbing and the 8 KB / 64 KB payload caps all apply). The writers keep +/// their own Apply call — redaction is idempotent, so the stored row is unaffected. +/// The copy is also capped by BYTES as well as by count, because it rides a gRPC reply whose +/// default 4 MB receive limit a few hundred large error rows would otherwise breach — and that +/// same reply carries the script's return value, so an oversized trace would fail a real call. /// public sealed class FlowTraceCollector { /// Default inline cap per run (design §5.4 MaxTraceEventsPerRun). public const int DefaultMaxEvents = 500; + /// + /// Default inline byte budget per run — the summed redacted + /// length, well under the 4 MB gRPC default receive limit the reply must fit in. + /// + public const int DefaultMaxBytes = 1_048_576; + private readonly List _events = new(); private readonly object _gate = new(); private readonly int _max; + private readonly int _maxBytes; + private readonly IAuditRedactor? _redactor; + private int _bytes; - /// True once an was dropped because the cap was reached. + /// + /// True once an was dropped because the event cap or the byte budget + /// was reached. + /// public bool Truncated { get; private set; } /// @@ -33,25 +53,41 @@ public sealed class FlowTraceCollector /// public bool Completed { get; private set; } - /// Creates a collector with the given inline cap (must be positive). - public FlowTraceCollector(int maxEvents = DefaultMaxEvents) + /// + /// Creates a collector with the given inline caps (both must be positive) and the redactor + /// the inline copy is filtered through. A null — a host that + /// never called AddAuditLog, or a test — collects raw but is still capped. + /// + public FlowTraceCollector( + int maxEvents = DefaultMaxEvents, + int maxBytes = DefaultMaxBytes, + IAuditRedactor? redactor = null) { if (maxEvents <= 0) throw new ArgumentOutOfRangeException(nameof(maxEvents), "must be positive"); + if (maxBytes <= 0) throw new ArgumentOutOfRangeException(nameof(maxBytes), "must be positive"); _max = maxEvents; + _maxBytes = maxBytes; + _redactor = redactor; } /// - /// Appends an event, or marks the collector truncated once the cap is reached. A no-op - /// after . + /// Appends the REDACTED copy of an event, or marks the collector truncated once the event + /// cap or the byte budget is reached. A no-op after . /// public void Add(AuditEvent evt) { ArgumentNullException.ThrowIfNull(evt); + // Redaction runs OUTSIDE the lock (arch-review F1): IAuditRedactor is a pure function + // but not necessarily a cheap one (regex scrubbing + payload truncation), and the + // collector is shared by every emit site of a run. + var filtered = _redactor is null ? evt : _redactor.Apply(evt); + var size = filtered.DetailsJson?.Length ?? 0; lock (_gate) { if (Completed) return; - if (_events.Count >= _max) { Truncated = true; return; } - _events.Add(evt); + if (_events.Count >= _max || _bytes + size > _maxBytes) { Truncated = true; return; } + _events.Add(filtered); + _bytes += size; } } diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs index 77b46ef1..6a4564d6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs @@ -247,7 +247,15 @@ internal static class ScriptRunLauncher // caller to reply to — a trace-armed trigger run (replyTo == Nobody) writes every row to the // audit writer for PullTraceEvents and would only keep a never-read list alive. The // ScriptRun row itself is emitted below on every exit path of a traced run. - var collector = trace && !replyTo.IsNobody() ? new FlowTraceCollector() : null; + // Arch-review F1: the inline copy is redacted at the collector, so the collector carries + // the host's IAuditRedactor. That is a SINGLETON (AddAuditLog), resolved here from the + // ROOT provider for the same reason siteEventLogger below is — the traced ScriptRun row + // is emitted from the catch blocks too, including the shed-at-dequeue path that throws + // before the DI scope exists. Null in tests / hosts without AddAuditLog: raw, still capped. + var collector = trace && !replyTo.IsNobody() + ? new FlowTraceCollector( + redactor: serviceProvider?.GetService()) + : null; var startedUtc = DateTime.UtcNow; ScriptRuntimeContext? context = null; IAuditWriter? auditWriter = null; diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/FlowTraceCollectorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Debugging/FlowTraceCollectorTests.cs similarity index 57% rename from tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/FlowTraceCollectorTests.cs rename to tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Debugging/FlowTraceCollectorTests.cs index a9b56210..d27d3176 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/FlowTraceCollectorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Debugging/FlowTraceCollectorTests.cs @@ -1,7 +1,7 @@ using ZB.MOM.WW.Audit; -using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Debugging; -namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Scripts; +namespace ZB.MOM.WW.ScadaBridge.Commons.Tests.Debugging; /// Flow Debugger design 2026-09-05 §5.4: the per-run inline trace sink. public class FlowTraceCollectorTests @@ -85,6 +85,56 @@ public class FlowTraceCollectorTests Assert.Equal(snapshot, c.Complete()); // idempotent } + /// + /// Arch-review F1: the inline copy must be the SAME filtered record the writer persists — + /// the collector applies the redactor itself rather than handing on the raw event. + /// + [Fact] + public void Add_AppliesTheRedactor_SoTheInlineCopyIsTheFilteredEvent() + { + var c = new FlowTraceCollector(redactor: new UpperRedactor()); + var evt = Evt(1) with { DetailsJson = "{\"secret\":\"abc\"}" }; + + c.Add(evt); + + var stored = Assert.Single(c.Complete()); + Assert.Equal("{\"SECRET\":\"ABC\"}", stored.DetailsJson); + Assert.NotSame(evt, stored); + } + + /// + /// Arch-review F1: the 500-event cap alone does not bound the gRPC reply the inline copy + /// rides on — a byte budget over the redacted DetailsJson truncates first. + /// + [Fact] + public void Add_OverByteBudget_Truncates_KeepsEarlierEvents() + { + var c = new FlowTraceCollector(maxEvents: 500, maxBytes: 100); + var big = Evt(1) with { DetailsJson = new string('x', 80) }; + var big2 = Evt(2) with { DetailsJson = new string('y', 80) }; + + c.Add(big); + c.Add(big2); + + Assert.True(c.Truncated); + Assert.Single(c.Complete()); + } + + /// The byte budget has a documented default and is validated like the event cap. + [Fact] + public void DefaultByteBudget_Is1Mb_AndNonPositiveBudgetIsRejected() + { + Assert.Equal(1_048_576, FlowTraceCollector.DefaultMaxBytes); + Assert.Throws(() => new FlowTraceCollector(maxBytes: 0)); + } + + /// Upper-cases DetailsJson so a test can see the filtered copy land. + private sealed class UpperRedactor : IAuditRedactor + { + public AuditEvent Apply(AuditEvent rawEvent) + => rawEvent with { DetailsJson = rawEvent.DetailsJson?.ToUpperInvariant() }; + } + private static AuditEvent NewEvent() => new() { EventId = Guid.NewGuid(), diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptRuntimeContextTraceEmissionTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptRuntimeContextTraceEmissionTests.cs index 718abd7a..267e6a70 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptRuntimeContextTraceEmissionTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptRuntimeContextTraceEmissionTests.cs @@ -209,6 +209,39 @@ public class ScriptRuntimeContextTraceEmissionTests : TestKit Assert.False(ctx.IsTraced); } + /// + /// Arch-review F1: the collector applies the host redactor, so the inline copy the reply + /// carries is the FILTERED record — while the writer still receives the raw event and + /// redacts it itself (redaction is idempotent, so the stored row is identical). + /// + [Fact] + public async Task SetAttribute_WhenTraced_InlineCopyIsRedacted_WriterStillSeesTheRawEvent() + { + var probe = CreateTestProbe(); + var writer = new CapturingAuditWriter(); + var collector = new FlowTraceCollector(redactor: new UpperRedactor()); + var ctx = CreateContext(probe.Ref, writer, traced: true, collector); + + var call = ctx.SetAttribute("Setpoint", "42"); + var cmd = probe.ExpectMsg(TimeSpan.FromSeconds(5)); + probe.Reply(new SetStaticAttributeResponse(cmd.CorrelationId, InstanceName, "Setpoint", true, null, DateTimeOffset.UtcNow)); + await call; + + var written = Assert.Single(writer.Events); + var collected = Assert.Single(collector.Complete()); + Assert.NotSame(written, collected); + Assert.Contains("\"channel\":\"ScriptTrace\"", written.DetailsJson); + Assert.NotEqual(written.DetailsJson, collected.DetailsJson); + Assert.Equal(written.DetailsJson!.ToUpperInvariant(), collected.DetailsJson); + } + + /// Upper-cases DetailsJson so a test can see the filtered copy land. + private sealed class UpperRedactor : IAuditRedactor + { + public AuditEvent Apply(AuditEvent rawEvent) + => rawEvent with { DetailsJson = rawEvent.DetailsJson?.ToUpperInvariant() }; + } + [Fact] public async Task SetAttribute_WhenTraced_FailedWrite_EmitsFailedRow_ThenThrows() {