ScadaBridge/docs/plans/2026-09-06-flow-debugger-arch-review.md

Flow Debugger — architectural / code review

Date: 2026-09-06 · Status: REVIEW REPORT (no code changed by this review) Scope: every commit from 7fa916e2 (Flow Debugger design) to 0ccbe140 (cached-call forwarder enqueue-and-observe) on main — Flow Debugger v1.0, v1.1 outbound calls, v1.2 follow-ups, the outcome-merge fix and the two same-day performance commits. 165 files, +20 228 / −778 lines. Method: read of every non-test source file in the range against the three design/plan documents (2026-09-05-flow-debugger-design.md, 2026-09-06-flow-debugger-outbound-calls.md, 2026-09-06-flow-debugger-followups.md) plus 2026-09-06-tracking-store-async-writes-design.md; targeted verification of the claims the code comments make (where redaction runs, how the ManagementActor dispatches, proto field numbering, timestamp storage format, page gates); full solution test run (§6).

1. Verdict

The feature is architecturally sound and lands where the design said it would: trace steps are audit events, every contract change is additive, the site never depends on central for tracing, the run path is one management command shared by UI and CLI, and the per-node actor is not blocked by a long run. Test coverage is broad (≈ 6 000 lines of new tests across every layer).

Four findings warrant a fix before the feature is relied on outside the rig, all Medium, none requiring a redesign:

#	Severity	Finding	Owner component
F1	Medium	The inline trace copy (routed reply + Live result) is the pre-redaction, un-capped event — per-target SQL parameter redaction and the 8 KB / 64 KB payload caps only apply to the stored row; the wire copy is bounded by count, not bytes	Site Runtime, Inbound API, Management
F2	Medium	The method-entry Live run has no site-scope check; only the value-change entry enforces PermittedSiteIds	Management, Inbound API
F3	Medium	Replay is Deployer-gated; the design (D6, §9) says OperationalAudit, and the Audit Log / Execution Chain drill-ins invite auditors onto a page they cannot open	Central UI, Management
F4	Medium	Three unrelated run timeouts: server = method timeout + 10 s; UI Ask = fixed 2 min; CLI = fixed 2 min. A method with TimeoutSeconds > ~110 makes both clients give up while the run continues	Central UI, CLI

Then nine Low findings (F5–F13) — ordering under clock skew, a trace-arm false positive, silent shedding of traced runs, continuation-scheduler consistency, a duplicated chain walker, a kind-only prediction overlay, always-on RouteCall vs SkipBodyCapture, dead stream fields, and doc/implementation drift — and a list of what was checked and found correct (§4).

2. Findings

Severity: High = data loss / security boundary / outage; Medium = wrong behaviour an operator will hit, or a documented property that does not hold; Low = quality, consistency, or a narrow edge. Each finding names the file, states what the code does, why it matters, and the fix.

F1 — Inline trace copy bypasses redaction and payload caps [Medium]

Where. SiteRuntime/Scripts/FlowTraceCollector.cs; ScriptRuntimeContext.Collect / EmitTrace; ScriptRunLauncher.EmitScriptRun; InboundAPI/RouteHelper.cs EmitRouteCall; InboundAPI/InboundDatabaseHelper.cs EmitAudit; ManagementService/FlowDebug/FlowDebugRunner.cs.

What. Every emit site deliberately hands the collector / trace sink the same AuditEvent instance it then hands the writer, "collector first". But redaction and capping live inside the writers — FallbackAuditWriter.WriteAsync (_redactor.Apply(evt), line 88) on the site and CentralAuditWriter.WriteAsync (line 113) on central — and both return a new filtered record. The collector therefore holds the raw event: no PerTargetOverrides SQL-parameter redaction, no header redaction, no DefaultCapBytes (8 KB) / ErrorCapBytes (64 KB) truncation. That raw list is what ScriptCallResult.TraceEvents → RouteToCallResponse.TraceEvents carries over gRPC, what FlowDebugRunResult.Events returns to the page and the CLI (--format json prints it verbatim), and what the page renders in a Live run. Replay reads the stored, redacted rows — so Live and Replay of the same execution can show different payloads.

Why it matters.

• Design §9 states "Trace payloads pass through the existing redaction policy (auth headers redacted, 8 KB cap, 64 KB on error rows)". It does not hold for the Live path.
• A per-target redaction override exists precisely to keep specific SQL parameter values out of audit; a Deployer running the Flow Debugger sees them anyway.
• Size: FlowTraceCollector caps at 500 events, not bytes. A traced run whose DB/API rows carry large request/response summaries (a 64 KB error detail × a few hundred rows) exceeds the gRPC default 4 MB receive limit on central's GrpcSiteTransport channel (no MaxReceiveMessageSize is set anywhere in src/). The routed call then fails with ResourceExhausted — and because the same reply carries the script's return value, a method flagged TraceAllCalls would fail a production call on payload size alone.

Fix. Apply the redactor at the collector boundary, not the writer: inject IAuditRedactor into ScriptRuntimeContext's Collect (site) and into FlowDebugRunner's sink (central) so the collector stores redactor.Apply(evt); the writers keep their own call (idempotent). Add a byte budget to FlowTraceCollector (e.g. 1 MB, counted on the redacted DetailsJson length) alongside the 500-event cap, setting Truncated when either trips. Tests: a per-target SQL redaction override is honoured on RouteToCallResponse.TraceEvents; a collector over budget truncates. Effort: small (≈ 60 lines + tests).

F2 — Method-entry Live run has no site-scope check [Medium]

Where. FlowDebugRunner.RunMethodAsync vs RunValueChangeAsync (EnforceSiteScope).

What. The value-change entry refuses with SiteScopeViolationException when the instance's site is outside user.PermittedSiteIds. The method entry has no equivalent: a Deployer whose role is scoped to site A can pick any method whose script routes to site B and run it Live — real Route.To().Call, SetAttributes, Database.*, Notify.Send against site B, with the operator's username on the inbound row. Design §9 only mentions the value-change entry, so this is an omission in the design carried into the code, not a deviation.

Why it matters. Site-scoped Deployer is the project's operator-of-one-plant model. The HTTP inbound API does not weaken it (API keys are a separate credential); the Flow Debugger does.

Fix. Cheapest and complete: give RouteHelper a site-scope predicate (WithSiteScope( IReadOnlySet<int> permittedSiteIds)), checked in RouteTarget right after _instanceLocator resolves the target's site id, throwing SiteScopeViolationException (already mapped to ManagementUnauthorized). FlowDebugRunner.RunMethodAsync sets it from user; the HTTP endpoint passes nothing (unchanged). The static prediction already resolves constant route targets, so the page can also warn before the run. Effort: small.

F3 — Replay requires Deployer; the design says OperationalAudit [Medium]

Where. ManagementActor.GetRequiredRoles (FlowDebugReplayCommand => DeployerOnly); FlowDebugger.razor ([Authorize(Policy = RequireDeployment)]); drill-in buttons in AuditEventDetail.razor and ExecutionTreePage.razor.

What. Design D6 / §9: "Live run is a Deployer act … Replay needs OperationalAudit". The implementation gates the page and all three commands on Deployer. The Audit Log and Execution Chain pages — whose own gate is OperationalAudit — now show an Open in Flow Debugger button that an auditor without Deployer cannot use.

Fix. Either amend D6 (and hide the drill-in for non-Deployers), or implement the split: page policy "OperationalAudit or Deployment", FlowDebugReplayCommand accepts either role, Run button and the two run commands stay Deployer-only. The split is ≈ 20 lines plus a RequiredRoleMatrixTests row; it is the better answer because the audit drill-in is the feature's main entry for a past incident.

F4 — Three unrelated Live-run timeouts [Medium]

Where. FlowDebugRunner.RunMethodAsync (timeout + RunGrace, CancellationToken.None from the actor); CentralUI/Services/FlowDebugService.cs (RunAskTimeout = 2 min); CLI/Commands/FlowDebugCommands.cs (RunTimeout = 2 min); ManagementEndpoints long-running list (server side is correctly exempted from the ordinary Ask timeout).

What. The server bounds a run by the method's TimeoutSeconds (unbounded positive int) plus 10 s. Both clients wait a fixed 2 minutes. FlowDebugService's comment asserts "both fit inside two minutes" — nothing enforces it. A method with TimeoutSeconds = 180 (legal; long integration handshakes exist) makes the page show TRANSPORT_ERROR and the CLI a 504 at 120 s while the run continues to completion on central, writes its audit rows, and cannot be cancelled (the handler runs on CancellationToken.None). The value-change entry has the same shape (WaitSeconds is user-supplied and unbounded; UI sends the default 45).

Fix. Derive the client wait from the request: UI = method.TimeoutSeconds + RunGrace + 15 s (the page already has the method list) and WaitSeconds + 15 s; CLI = same after a GetApiMethod read, or a --timeout option. Optionally clamp TimeoutSeconds for Live runs and say so in the error. Effort: small.

F5 — Step ordering trusts cross-node timestamps [Low]

Where. FlowDebugRunner.RunMethodAsync (events.OrderBy(e => e.OccurredAtUtc)), FlowGraphBuilder.Build (global sort by OccurredAtUtc, spawner search p.OccurredAtUtc <= first.OccurredAtUtc).

What. Central rows carry central's clock, site rows the site's. The collector preserved emission order, and the runner discards it by re-sorting. With a site clock 1–2 s ahead, site steps sort after the Response node in Steps (the CLI table and the detail list) and a trigger cascade's spawner search falls back to parentChain[0]. Edges are id-based so the graph stays connected; this is display order only. Route calls are unaffected (linked by ChildExecutionId).

Fix. Keep a sequence number per event on the inline path (the collector's index) and sort by (OccurredAtUtc within an execution, sequence); or simply preserve the collector's order and merge Replay rows behind it. Low.

F6 — Trace arm can stamp an unrelated device update [Low]

Where. InstanceActor.HandleSetDataAttribute (arms before the DCL write completes), HandleTagValueUpdate (consumes the arm "whatever the value").

What. The arm is recorded when the traced write is issued, not when the DCL acknowledges it. If the write fails (bad quality, device rejects) the arm stays for 30 s; the next unrelated subscription update on that attribute is stamped with the debugger's execution id and spawns a traced, parented run — the debugger then shows a "device echo" of a write that never landed. Design §5.3 accepts "whatever the value", which covers clamping, not failure.

Fix. Clear the arm in the WriteTagResponse failure continuation (the actor already pipes the response back; a Tell to self with the attribute name is enough). Low.

F7 — A shed or rate-limited traced run is invisible [Low]

Where. ScriptActor.TrySpawnExecution (MinTimeBetweenRuns suppression) and SpawnExecution (MaxConcurrentRunsPerScript → ShedRun).

What. A traced value-change run dropped by either guard emits no ScriptTrace row; the debugger reports NO_TRIGGER_WITHIN_WINDOW with the message "no value-change trigger on this attribute, or no device echo" — both wrong in that case, and the operator is sent to check the wrong things.

Fix. When trace is set, emit a ScriptRun row with Failed and an error of shed: MaxConcurrentRunsPerScript / suppressed: MinTimeBetweenRuns (the launcher's EmitScriptRun can be reused with elapsedMs = 0). Low.

F8 — Fault continuations: scheduler and logging inconsistent [Low]

Where. ScriptRuntimeContext.EmitTrace, ScriptRunLauncher.EmitScriptRun, RouteTarget.EmitRouteCall, InboundDatabaseHelper.EmitAudit.

What. All four use ContinueWith(..., TaskContinuationOptions.OnlyOnFaulted) with no scheduler argument, so the continuation inherits TaskScheduler.Current — on a script thread that is the blocking-I/O ScriptExecutionScheduler. CachedCallTelemetryForwarder (same commit range) passes TaskScheduler.Default explicitly. The two central emitters also discard the fault (static _ => { }) with no log line, so a central audit outage is silent on those rows.

Fix. Pass CancellationToken.None, OnlyOnFaulted | ExecuteSynchronously, TaskScheduler.Default everywhere, and log at Warning in the two central emitters (a logger is one constructor parameter away in both). Low.

F9 — GetTraceEventsAsync duplicates the execution-tree walk [Low]

Where. ConfigurationDatabase/Repositories/AuditLogRepository.cs.

What. The new method re-implements GetExecutionTreeAsync's three-statement walk (up to root, root window, recursive Edges/Chain CTE with MAXRECURSION and the 7-day window) as a second copy, then filters channels with EF. Two walkers that must stay in step (window slack, depth, the DISTINCT-edges trick).

Fix. Extract ResolveChainExecutionIdsAsync(executionId) and have both call it. Low.

F10 — Prediction overlay matches real steps by kind only [Low]

Where. FlowGraphView.ComputeLayout (unmatched.FirstOrDefault(p => p.Kind == s.Kind)).

What. A method with two route calls (A.X then B.Y) that only executes the second one matches the real B.Y step to the predicted A.X node; the ghost shown is B.Y. Wrong greying for multi-route or branching methods.

Fix. Match by (Kind, Title) first, then fall back to kind. Low, UI only.

F11 — Always-on RouteCall row ignores the method's SkipBodyCapture [Low]

Where. RouteTarget.EmitRouteCall (requestSummary: TraceJson(args)), vs AuditWriteMiddleware (honours PerTargetOverrides[methodName].SkipBodyCapture on the inbound row).

What. An operator who configured SkipBodyCapture for a method to keep its body out of the audit log now gets the routed arguments — derived from that body — on every call's RouteCall row, keyed by {instance}.{script}, which the per-method override does not match. Also: every routed call is now one extra central MS SQL insert (fire-and-forget); at inbound-driven rates that doubles the per-call central audit write, which the design accepted (D5) but should be stated in the volume section.

Fix. Thread the method's SkipBodyCapture into RouteHelper (the executor has the method) and emit the row without summaries when set; add the sentence to design §10. Low.

F12 — Shipped but unused: AttributeValueUpdate.source_execution_id / trace [Low]

Where. sitestream.proto fields 7/8, StreamRelayActor, SiteStreamGrpcClient.

What. Design §5.4 planned the value-change entry to subscribe to the debug stream and take the spawned id from the stamped AttributeValueChanged. The implementation polls PullTraceEvents by ParentExecutionId instead (simpler, and correct). The stream fields are therefore populated on every change and consumed by nothing. Not harmful — additive, one string per update — but the design should record the switch and the fields should be marked "reserved for the debug-stream path" or dropped from the client mapping.

F13 — Documentation drift to reconcile [Low]

• Design §5.4 (debug-stream subscription) vs the polling implementation — see F12.
• Design D6 / §9 (Replay = OperationalAudit) vs Deployer everywhere — see F3.
• Design §9 (redaction applies to trace payloads) vs F1.
• ApiMethodForm / CLI help say "~7 audit rows per call"; the outbound-calls addendum makes a call with one cached DB write and one notification 10 rows (3 trace-only). Say "7–10".
• FlowDebugRunner.RunMethodAsync writes the inbound row after the run, so a Replay taken during a long Live run shows only the RouteCall. Fine, but not stated anywhere.
• SchemaRefResolver became public for the runner; note it in Component-InboundAPI or move the validation into InboundScriptExecutor.

3. Performance commits (tracking store + forwarder)

OperationTrackingStore single-writer queue (d91135f9) — reviewed line by line, no findings: _pending is balanced on every path (enqueue failure, batch complete, per-row fallback, pump teardown); the read barrier is skipped only when nothing is queued and otherwise rides the same FIFO, so read-your-writes holds; the per-row fallback keeps good rows; DisposeAsync drains and sync Dispose() fails queued items as documented; timestamps are stamped at record time. The one trade — a queued-but-unapplied row lost on a hard crash — is stated in the file and the design memo.

CachedCallTelemetryForwarder enqueue-and-observe (0ccbe140) — correct, and the design memo's "correction" section is the right record: the first attribution (SQLite commits) was wrong, the measurement said so, and the doc says so. The measured result (inbound 194 → 25 ms, script run 176 → 17 ms) stands.

4. Verified correct (not findings)

• Additive contracts throughout. Every record gains trailing optional parameters; proto fields are new numbers (RouteToCallRequestDto.trace = 7, RouteToCallResponseDto 6/7, RouteToSetAttributesRequestDto.trace = 6, AttributeValueUpdate 7/8, new PullTraceEvents RPC); AuditChannel.ScriptTrace and the four kinds are appended last; UpdateApiMethodCommand. TraceAllCalls is bool? so an older CLI never resets it; proto3 defaults are the negotiation, as for stream batching. The SiteCommandDtoMapper maps an empty trace_events to null.
• The ManagementActor is not blocked by a Live run — HandleEnvelope runs ProcessCommand and PipeTos the result, so a 5-minute run does not stall other management commands. The standby refusal (IActiveNodeGate, STANDBY_NODE) and the new ManagementCommandException( message, code) path are correct and backwards compatible.
• FlowTraceCollector.Complete() closes the sink at the reply snapshot; later lifecycle rows go to the writer only; GuardTraceSink means a throwing observer can neither fail a call nor skip the durable write (pinned by tests).
• PullTraceEvents sits behind the existing /sitestream.SiteStreamService/ PSK interceptor prefix; caps ids at 64 and rows at 2000 on both ends; moves no forward-state cursor; the new IX_audit_event_occurred index bounds the json_extract scan and the timestamp text format matches the stored "o" form.
• Migration AddApiMethodTraceAllCalls + manual SQL twin with the QUOTED_IDENTIFIER header.
• Central Database.* audit (InboundDatabaseHelper): one DbOutbound/DbWrite row per call, vocabulary-locked with the site's AuditingDbCommand; failure inside the audited span; result or exception always reaches the script.
• Graph builder: fold by (kind, CorrelationId), LifecycleRank tie-break, IsAsync rule, leading-async linking from the spawner, stable synthesized Response id — all pinned by FlowGraphBuilderTests.
• Page: outcome refresh merges by EventId; the pending-refresh loop is bounded (5 s / 2 min), cancelled on mode switch, new run and dispose; Retry/Discard re-check site scope and reuse the report pages' relay and wording; the report pages are RequireDeployment too, so the debugger's actions add no privilege. The prediction circuit deadlock is fixed with a resolve-and-rewalk loop and pinned by a single-thread-pump test.
• Site-side gating: untraced runs allocate no collector and emit nothing; only traced SetStaticAttributeCommand writes arm; TraceArmWindowSeconds is validated > 0.
• Rig proof: verify-delmia-notifier.sh steps H and I (live + replay, outbound nodes) passed on the rig per the commit ledger.

5. Recommended order

1. F1 (redact + byte-cap the inline copy) and F2 (route-time site scope) — same session, both small, both close a stated security property.
2. F3 (Replay role split) and F4 (derive client waits) — small, operator-visible.
3. F6, F7, F8 — site-runtime polish, one commit.
4. F5, F9, F10, F11 — quality; can ride the next Flow Debugger change.
5. F12, F13 — docs; do with (1) so the design stops disagreeing with the code.

Register rows: F5, F10, F11, F12 are candidates for the deferred-work register if not fixed in the next pass; F1–F4 should not be deferred.

6. Test evidence

Full dotnet test ZB.MOM.WW.ScadaBridge.slnx at review time (2026-09-06, main @ 0ccbe140): 29 of 30 projects green, 9 297 tests passed. The one red project is CentralUI.PlaywrightTests (164 / 175): its 11 failures are the role-negative browser checks (RoleNavigationTests.*_DoesNotSee*, AuditLogPageTests.PermissionGating_…_IsDeniedAccess, LoginTests.UnauthenticatedUser_RedirectsToLogin) — every one asserts that a lesser role is denied something, which cannot hold on the rig since 2026-09-06 (2b80e1d8) because both central nodes run DisableLogin=true and every browser session is the all-roles multi-role user. None touches the Flow Debugger; Deployment/FlowDebuggerTests (Live run + localStorage restore) passed. Consequence worth recording alongside the login-disabled decision: the rig can no longer prove role gating in the browser, so the Playwright suite's role-negative tests need either a second, login-enabled central to target or a fixture that overrides the identity per test.