docs(plans): Flow Debugger architectural/code review — 4 Medium + 9 Low findings, test evidence
Full review of the Flow Debugger commit range (v1.0, v1.1 outbound calls, v1.2 follow-ups, outcome-merge fix, two perf commits). Also records the exact 11 role-negative Playwright tests that cannot pass on the login-disabled rig in the cluster-ops skill. Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
This commit is contained in:
Joseph Doherty
@@ -6,7 +6,7 @@ description: How to drive the local ScadaBridge docker cluster — CLI managemen
|
||||
# ScadaBridge Cluster Ops (Docker / OrbStack)
|
||||
|
||||
- **Management URL**: `http://localhost:9000` — the CLI connects via the Traefik load balancer, which routes to the active central node. Direct access: central-a on port 9001, central-b on port 9002.
|
||||
- **Central UI login is DISABLED on the rig (both central nodes, since 2026-09-06)** — `ScadaBridge:Security:Auth:DisableLogin=true` in `docker/central-node-{a,b}/appsettings.Central.json`, so `http://localhost:9000` (and 9001/9002) opens the UI as `multi-role` with every role, no LDAP login. The CLI/management API still takes `--username/--password` (Basic auth is a separate path), and `POST /auth/login` still answers 302, so the Playwright fixture login keeps working; only `LoginTests` (expects a `/login` redirect + 401 on bad credentials) fails against this rig. Flip the two keys back to `false` and recreate the node for real-login checks.
|
||||
- **Central UI login is DISABLED on the rig (both central nodes, since 2026-09-06)** — `ScadaBridge:Security:Auth:DisableLogin=true` in `docker/central-node-{a,b}/appsettings.Central.json`, so `http://localhost:9000` (and 9001/9002) opens the UI as `multi-role` with every role, no LDAP login. The CLI/management API still takes `--username/--password` (Basic auth is a separate path), and `POST /auth/login` still answers 302, so the Playwright fixture login keeps working. Expected Playwright failures on this rig are exactly the 11 role-NEGATIVE tests (full run 2026-09-06: 164/175) — `RoleNavigationTests.*_DoesNotSee*`, `AuditLogPageTests.PermissionGating_DesignerWithoutOperationalAudit_IsDeniedAccess`, `LoginTests.UnauthenticatedUser_RedirectsToLogin` — because every browser session carries every role; any other Playwright failure is real. Flip the two keys back to `false` and recreate the node for real-login checks.
|
||||
- **Test user**: `--username multi-role --password password` — has Admin, Design, and Deployment roles. The `admin` user only has the Admin role and cannot create templates, data connections, or deploy.
|
||||
- **Config file**: `~/.scadabridge/config.json` — stores `managementUrl` and default format. See `docker/README.md` for a ready-to-use test config.
|
||||
- **Rebuild cluster**: `bash docker/deploy.sh` — builds the `scadabridge:latest` image and recreates all containers. Run this after code changes to ManagementActor, Host, or any server-side component.
|
||||
|
||||
@@ -0,0 +1,330 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user