# 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 `PipeTo`s 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.