e15aa0e430
The Flow Debugger design doc still described the pre-remediation shape: a
debug-stream subscription for the value-change entry, Deployer everywhere, an
unredacted inline trace copy and "~7 audit rows per call". Reconciled with what
shipped on this branch (Tasks 1-12):
- design D6 / §5.4 / §9 / §10: RequireFlowDebugger page policy with Deployer-only
execution (F3), the PullTraceEvents polling path with AttributeValueUpdate
fields 7/8 recorded as RESERVED and unconsumed (F12), the redacting + byte-
capped shared FlowTraceCollector (F1), route-time site scope (F2), the derived
client waits (F4), the 7-10 row estimate, the doubled central audit write per
routed call, and the inbound-row-after-the-run ordering.
- sitestream.proto: fields 7/8 marked reserved-but-populated, numbers not to be
reused (comment only; no regeneration needed).
- Component-InboundAPI: SkipBodyCapture on the RouteCall row (F11),
SchemaRefResolver's public visibility, WithSiteScope / SITE_SCOPE_DENIED (F2),
Replay's wider role set (F3).
- Component-CentralUI: page policy vs Deployer-only controls (F3), route-time
site scope (F2), client wait derivation (F4).
- "7-10 audit rows per call (3 of them trace-only)" everywhere the estimate is
quoted: the method form, both CLI --trace-all-calls help strings, the CLI
README, the ApiMethod entity doc, the manual migration script and the two
Flow Debugger plans.
- arch-review doc: new §7 Resolution listing F1-F13 with the fixing commit, plus
the tracking-store test race fixed alongside them (86451382).
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
359 lines
27 KiB
Markdown
359 lines
27 KiB
Markdown
# 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 understate the row count (they were written before the outbound-calls
|
||
addendum, which makes a call with one cached DB write and one notification **10 rows**, 3 of them
|
||
trace-only). Say "7–10 audit rows per call".
|
||
- `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.
|
||
|
||
## 7. Resolution
|
||
|
||
All thirteen findings were remediated on branch `fix/flow-debugger-arch-review` (plan
|
||
`docs/plans/2026-09-06-flow-debugger-arch-review-remediation.md`, 2026-09-06). Nothing was
|
||
deferred to the register.
|
||
|
||
| # | Outcome | Commit |
|
||
|---|---------|--------|
|
||
| F1 | `FlowTraceCollector` moved to Commons `Types/Debugging/`, applies `IAuditRedactor` inside `Add` and caps at 500 events **and** 1 MB of summed `DetailsJson`; the site launcher resolves the redactor from the run's scope, and the central `FlowDebugRunner` collects through the same instance and reports its `Truncated`. | `e79452d2` fix(site-runtime): Flow Debugger inline trace copy is redacted and byte-capped at the collector (arch-review F1, site half) · `def6bab3` fix(management): Flow Debugger Live result is the redacted, capped trace copy (arch-review F1, central half) |
|
||
| F2 | `RouteHelper.WithSiteScope(IReadOnlySet<string>?)` refuses an out-of-scope target site before any site traffic (`RouteSiteScopeException` → executor code `SITE_SCOPE_DENIED`); the runner rethrows it as `SiteScopeViolationException` (`ManagementUnauthorized`) **after** writing the inbound audit row. Administrators and unscoped users are exempt; the HTTP endpoint is unchanged. | `20a05941` fix(inbound-api): Flow Debugger method-entry Live run enforces the operator's site scope at route time (arch-review F2) |
|
||
| F3 | New `AuthorizationPolicies.RequireFlowDebugger` (`FlowDebuggerAccessRoles` = Deployer, Administrator, Viewer) gates the page, and `FlowDebugReplayCommand` accepts the same set; the two run commands, the Live controls, Retry/Discard and the `?methodId=`/`?instanceId=` drill-ins stay Deployer-only, with the page pinning Replay mode for a non-Deployer. | `f86eb61d` fix(central-ui): Flow Debugger Replay opens to OperationalAudit roles; Live run and Retry/Discard stay Deployer-only (arch-review F3) |
|
||
| F4 | Client waits derive from the server's bound: `RunAskTimeoutFor(timeoutSeconds)` = method timeout + `RunGrace` + 15 s slack (floor 30 s), `ValueChangeAskTimeoutFor(wait)` = wait + 15 s; the CLI gains `run-method --timeout-seconds` (default 120) and waits `--wait-seconds + 25` on `run-value-change`. | `97b0a4ba` fix(central-ui,cli): Flow Debugger client waits derive from the method timeout / wait window instead of a fixed 2 minutes (arch-review F4) |
|
||
| F5 | The graph orders executions by emission sequence and uses timestamps only **within** one execution, so a site clock offset can no longer reorder the swimlane; the Live runner keeps the collector's emission order. | `1dfb9cee` fix(commons): Flow graph orders executions by emission sequence, timestamps only within one clock (arch-review F5) |
|
||
| F6 | A failed traced DCL write disarms the trace arm (`DisarmTrace` self-message), so an unrelated device update can no longer consume it. | `50bf9ac7` fix(site-runtime): trace arm is cleared when the traced DCL write fails (arch-review F6) |
|
||
| F7 | A traced run shed by `MaxConcurrentRunsPerScript` or suppressed by `MinTimeBetweenRuns` emits a Failed `ScriptRun` row (`shed: MaxConcurrentRunsPerScript …` / `suppressed: MinTimeBetweenRuns …`) instead of vanishing from the graph. | `105541b6` fix(site-runtime): shed or rate-limited traced runs emit a Failed ScriptRun row (arch-review F7) |
|
||
| F8 | All four trace/outbound fault continuations run on `TaskScheduler.Default`, and the central emitters log a Warning on a failed audit write. | `262de38c` fix(audit): trace/outbound fault continuations run on the default scheduler and central emitters log (arch-review F8) |
|
||
| F9 | `GetTraceEventsAsync` and `GetExecutionTreeAsync` share one chain walker in `AuditLogRepository`. | `297ae56b` refactor(config-db): GetTraceEventsAsync and GetExecutionTreeAsync share one chain walker (arch-review F9) |
|
||
| F10 | The prediction overlay matches a real step by `(Kind, Title)` first, falling back to kind only, so two predicted nodes of the same kind no longer collapse onto one another. | `688136de` fix(central-ui): Flow Debugger prediction overlay matches by kind and title (arch-review F10) |
|
||
| F11 | The always-on `RouteCall` row honours the method's `SkipBodyCapture` — the executor builds the helper with `RouteHelper.WithBodyCapture(false)`; the row is still written, metadata only. | `453c3dec` fix(inbound-api): RouteCall audit row honours the method's SkipBodyCapture override (arch-review F11) |
|
||
| F12 | `AttributeValueUpdate.source_execution_id` / `trace` (fields 7/8) are **kept, populated and unconsumed** — documented as reserved for a future debug-stream path in `sitestream.proto` and design §5.4, with the field numbers pinned against reuse. Not removed: dropping them would be a wire change for no gain. | this commit |
|
||
| F13 | Design D6 / §5.4 / §9 / §10, `Component-CentralUI.md`, `Component-InboundAPI.md`, `CLAUDE.md`, the proto comment and the CLI/UI help strings reconciled with the shipped code; the trace-row estimate is "7–10 audit rows per call" everywhere; `SchemaRefResolver`'s `public` visibility and the inbound-row-after-the-run ordering are now stated. | this commit — docs: Flow Debugger design reconciled with the implementation (arch-review F12, F13) |
|
||
|
||
One test defect surfaced by §3's performance commits was fixed on the same branch:
|
||
`OperationTrackingStoreTests.ReadChangedSince_BoundaryTimestamp…` raced the new single-writer
|
||
queue (a missing `FlushAsync` before the test's direct `UPDATE`) — `86451382` test(site-runtime):
|
||
flush the tracking-store queue before the boundary-timestamp test's direct UPDATE.
|