fix(site-runtime): Flow Debugger inline trace copy is redacted and byte-capped at the collector (arch-review F1, site half)
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
This commit is contained in:
+44
-8
@@ -1,6 +1,6 @@
|
|||||||
using ZB.MOM.WW.Audit;
|
using ZB.MOM.WW.Audit;
|
||||||
|
|
||||||
namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts;
|
namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Debugging;
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Per-run sink for <c>ScriptTrace</c> audit events so a traced routed call can return them
|
/// Per-run sink for <c>ScriptTrace</c> 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
|
/// 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
|
/// minutes later, or an un-awaited call finishing after the run returned, must not mutate a
|
||||||
/// list the reply has already been built from. <see cref="Add"/> is a no-op once completed.
|
/// list the reply has already been built from. <see cref="Add"/> is a no-op once completed.
|
||||||
|
/// Arch-review F1 (2026-09-06): the inline copy is <b>REDACTED</b> — the collector applies the
|
||||||
|
/// host's <see cref="IAuditRedactor"/> 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 <c>Apply</c> 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.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public sealed class FlowTraceCollector
|
public sealed class FlowTraceCollector
|
||||||
{
|
{
|
||||||
/// <summary>Default inline cap per run (design §5.4 <c>MaxTraceEventsPerRun</c>).</summary>
|
/// <summary>Default inline cap per run (design §5.4 <c>MaxTraceEventsPerRun</c>).</summary>
|
||||||
public const int DefaultMaxEvents = 500;
|
public const int DefaultMaxEvents = 500;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Default inline byte budget per run — the summed redacted <see cref="AuditEvent.DetailsJson"/>
|
||||||
|
/// length, well under the 4 MB gRPC default receive limit the reply must fit in.
|
||||||
|
/// </summary>
|
||||||
|
public const int DefaultMaxBytes = 1_048_576;
|
||||||
|
|
||||||
private readonly List<AuditEvent> _events = new();
|
private readonly List<AuditEvent> _events = new();
|
||||||
private readonly object _gate = new();
|
private readonly object _gate = new();
|
||||||
private readonly int _max;
|
private readonly int _max;
|
||||||
|
private readonly int _maxBytes;
|
||||||
|
private readonly IAuditRedactor? _redactor;
|
||||||
|
private int _bytes;
|
||||||
|
|
||||||
/// <summary>True once an <see cref="Add"/> was dropped because the cap was reached.</summary>
|
/// <summary>
|
||||||
|
/// True once an <see cref="Add"/> was dropped because the event cap or the byte budget
|
||||||
|
/// was reached.
|
||||||
|
/// </summary>
|
||||||
public bool Truncated { get; private set; }
|
public bool Truncated { get; private set; }
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
@@ -33,25 +53,41 @@ public sealed class FlowTraceCollector
|
|||||||
/// </summary>
|
/// </summary>
|
||||||
public bool Completed { get; private set; }
|
public bool Completed { get; private set; }
|
||||||
|
|
||||||
/// <summary>Creates a collector with the given inline cap (must be positive).</summary>
|
/// <summary>
|
||||||
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 <paramref name="redactor"/> — a host that
|
||||||
|
/// never called <c>AddAuditLog</c>, or a test — collects raw but is still capped.
|
||||||
|
/// </summary>
|
||||||
|
public FlowTraceCollector(
|
||||||
|
int maxEvents = DefaultMaxEvents,
|
||||||
|
int maxBytes = DefaultMaxBytes,
|
||||||
|
IAuditRedactor? redactor = null)
|
||||||
{
|
{
|
||||||
if (maxEvents <= 0) throw new ArgumentOutOfRangeException(nameof(maxEvents), "must be positive");
|
if (maxEvents <= 0) throw new ArgumentOutOfRangeException(nameof(maxEvents), "must be positive");
|
||||||
|
if (maxBytes <= 0) throw new ArgumentOutOfRangeException(nameof(maxBytes), "must be positive");
|
||||||
_max = maxEvents;
|
_max = maxEvents;
|
||||||
|
_maxBytes = maxBytes;
|
||||||
|
_redactor = redactor;
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Appends an event, or marks the collector truncated once the cap is reached. A no-op
|
/// Appends the REDACTED copy of an event, or marks the collector truncated once the event
|
||||||
/// after <see cref="Complete"/>.
|
/// cap or the byte budget is reached. A no-op after <see cref="Complete"/>.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public void Add(AuditEvent evt)
|
public void Add(AuditEvent evt)
|
||||||
{
|
{
|
||||||
ArgumentNullException.ThrowIfNull(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)
|
lock (_gate)
|
||||||
{
|
{
|
||||||
if (Completed) return;
|
if (Completed) return;
|
||||||
if (_events.Count >= _max) { Truncated = true; return; }
|
if (_events.Count >= _max || _bytes + size > _maxBytes) { Truncated = true; return; }
|
||||||
_events.Add(evt);
|
_events.Add(filtered);
|
||||||
|
_bytes += size;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -247,7 +247,15 @@ internal static class ScriptRunLauncher
|
|||||||
// caller to reply to — a trace-armed trigger run (replyTo == Nobody) writes every row to the
|
// 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
|
// 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.
|
// 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<ZB.MOM.WW.Audit.IAuditRedactor>())
|
||||||
|
: null;
|
||||||
var startedUtc = DateTime.UtcNow;
|
var startedUtc = DateTime.UtcNow;
|
||||||
ScriptRuntimeContext? context = null;
|
ScriptRuntimeContext? context = null;
|
||||||
IAuditWriter? auditWriter = null;
|
IAuditWriter? auditWriter = null;
|
||||||
|
|||||||
+52
-2
@@ -1,7 +1,7 @@
|
|||||||
using ZB.MOM.WW.Audit;
|
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;
|
||||||
|
|
||||||
/// <summary>Flow Debugger design 2026-09-05 §5.4: the per-run inline trace sink.</summary>
|
/// <summary>Flow Debugger design 2026-09-05 §5.4: the per-run inline trace sink.</summary>
|
||||||
public class FlowTraceCollectorTests
|
public class FlowTraceCollectorTests
|
||||||
@@ -85,6 +85,56 @@ public class FlowTraceCollectorTests
|
|||||||
Assert.Equal(snapshot, c.Complete()); // idempotent
|
Assert.Equal(snapshot, c.Complete()); // idempotent
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// 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.
|
||||||
|
/// </summary>
|
||||||
|
[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);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// 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 <c>DetailsJson</c> truncates first.
|
||||||
|
/// </summary>
|
||||||
|
[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());
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>The byte budget has a documented default and is validated like the event cap.</summary>
|
||||||
|
[Fact]
|
||||||
|
public void DefaultByteBudget_Is1Mb_AndNonPositiveBudgetIsRejected()
|
||||||
|
{
|
||||||
|
Assert.Equal(1_048_576, FlowTraceCollector.DefaultMaxBytes);
|
||||||
|
Assert.Throws<ArgumentOutOfRangeException>(() => new FlowTraceCollector(maxBytes: 0));
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>Upper-cases <c>DetailsJson</c> so a test can see the filtered copy land.</summary>
|
||||||
|
private sealed class UpperRedactor : IAuditRedactor
|
||||||
|
{
|
||||||
|
public AuditEvent Apply(AuditEvent rawEvent)
|
||||||
|
=> rawEvent with { DetailsJson = rawEvent.DetailsJson?.ToUpperInvariant() };
|
||||||
|
}
|
||||||
|
|
||||||
private static AuditEvent NewEvent() => new()
|
private static AuditEvent NewEvent() => new()
|
||||||
{
|
{
|
||||||
EventId = Guid.NewGuid(),
|
EventId = Guid.NewGuid(),
|
||||||
+33
@@ -209,6 +209,39 @@ public class ScriptRuntimeContextTraceEmissionTests : TestKit
|
|||||||
Assert.False(ctx.IsTraced);
|
Assert.False(ctx.IsTraced);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// 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).
|
||||||
|
/// </summary>
|
||||||
|
[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<SetStaticAttributeCommand>(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);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>Upper-cases <c>DetailsJson</c> so a test can see the filtered copy land.</summary>
|
||||||
|
private sealed class UpperRedactor : IAuditRedactor
|
||||||
|
{
|
||||||
|
public AuditEvent Apply(AuditEvent rawEvent)
|
||||||
|
=> rawEvent with { DetailsJson = rawEvent.DetailsJson?.ToUpperInvariant() };
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task SetAttribute_WhenTraced_FailedWrite_EmitsFailedRow_ThenThrows()
|
public async Task SetAttribute_WhenTraced_FailedWrite_EmitsFailedRow_ThenThrows()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user