Commit Graph

10 Commits

Author SHA1 Message Date
Joseph Doherty f40409a688 docs: strip task-tracking IDs from code comments and fill missing XML docs
ci / nightly-windev (push) Has been skipped
ci / windows-x86 (push) Successful in 1m16s
ci / java (push) Successful in 2m5s
ci / portable (push) Failing after 5m9s
Tracking IDs are review bookkeeping that rots in shipped source; the rationale they tagged is kept. Comment-only change (CommentChecker 458 -> 16 false positives).
2026-10-07 15:34:32 -04:00
Joseph Doherty 7ec0b3594c fix(dashboard): close AttachEventsAsync re-entrancy window; pin ACL decision-table corners (SEC-25 review)
Follow-up to the per-session event ACL. Part of that change rode into 693a78d
via a concurrent agent's pathspec-less commit; this commit carries the review
fixes and uses pathspecs on the commit itself so it cannot recur in either
direction.

Gating the page's subscribe seam made AttachEvents asynchronous — it awaits the
authentication state — and that await is a suspension point the synchronous
version did not have. On a rapid A -> B navigation the suspended A continuation
resumes after B's parameter set has run to completion, re-reads the live
SessionId (now B's), and attaches B a SECOND time. The ACL is not bypassed —
the newer attach already cleared that same session — but the fields holding B's
first subscription are overwritten in place, so nothing ever disposes it: its
EventsHubViewerRegistry entry is never released, which keeps the mirror cloning
events for a session the page is no longer watching through that handle, and
its pump is never cancelled. A resource leak the ACL work introduced.

OnParametersSetAsync now claims a monotonic _attachGeneration synchronously,
before its first await, and AttachEventsAsync re-checks it after the await and
before any field write or Subscribe call. A stale attach returns rather than
detaching: it owns nothing, and tearing down there would destroy the newer
attach's subscription. DetachEventsAsync needs no such guard — it captures and
nulls the live fields synchronously before it awaits, so a resumed detach only
unwinds what it already took ownership of. Same dispatcher-owned identity idea
as the existing ReferenceEquals guards in PumpEventsAsync and
MarkDisconnectedAsync, one level up.

The interleaving is not expressible with the static HtmlRenderer idiom the other
page tests use: it renders a root component once and exposes no parameter-update
seam. The new test therefore adds a minimal Renderer subclass whose only job is
to mount a component and drive a second SetParametersAsync into it while the
first is parked on a gated AuthenticationStateProvider. That subclass is the
lone reason for a narrowly scoped BL0006 suppression, justified in place: it is
test-only scaffolding that never ships, and the cost of the warning coming true
is a compile break in one test file on an SDK bump. Confirmed non-vacuous by
mutation — with the generation check disabled the test goes red on the doubled
subscription and the two passing ACL tests stay green.

Two decision-table corners are now pinned rather than implied. Admin x
nonexistent session id resolves to ALLOW, because the admin bypass is evaluated
before the registry lookup; a plausible "look the session up first, it reads
better" refactor would flip it, so a test documents the ordering. EventsHub's
remarks said "an unknown session id is denied" without qualification, which read
as universal; they now state that the bypass is checked first and every rule
below it is a non-Admin rule.

HubTokenServiceTests gains the truly-absent-field case: a hand-built payload
JSON with no Tags key at all, protected through the same purpose, which is the
shape every in-flight token has across the deploy that introduces the field. The
existing test covered present-but-empty, which does not exercise the null
coalesce that stands between a legacy token and a crash on the hub auth path.
ProtectorPurpose became internal so the test cannot drift from the real purpose
string.

Tag-count cardinality cap considered and recorded as a deliberate non-goal.

Build 0 warnings / 0 errors; 48 filtered (ACL/hub/token/page) and 257 dashboard
tests pass.
2026-08-17 04:35:59 -04:00
Joseph Doherty 693a78db7d feat(alarms): structural degraded-status signal for truncated alarm snapshots
The truncation-cliff fix made alarm transitions truncation-safe but silent:
when GetXmlCurrentAlarms2 returns exactly maxAlmCnt records the worker
suppresses absence-implies-Clear inference and says so only in a rate-limited
stderr warning. No client and no operator could tell a complete active set
from a capped one.

Two additive proto3 booleans carry the verdict out:

- QueryActiveAlarmsReplyPayload.snapshot_truncated = 2 (worker IPC reply)
- ActiveAlarmSnapshot.from_truncated_snapshot = 16 (per record)

The per-record field is not an aesthetic choice. QueryActiveAlarms returns a
bare `stream ActiveAlarmSnapshot` with no envelope, header, or trailer, so a
per-record boolean is the only carrier that stays wire-compatible; an envelope
message would change every existing client's stream element type. The reply
payload states it too because a prefix filter can leave zero records and a
truncated fetch with nothing to report still has to say so. The flag means
"this set may be incomplete", never "this record is unreliable" — it is
independent of the subtag-fallback `degraded` field.

Detection is deliberately UNCHANGED: IsTruncatedFetch remains
`fetchedRecordCount >= maxAlarmsPerFetch`. The live probe (docs/AlarmProbeFindings.md,
ce5d8ae) could not verify whether ALARM_RECORDS/@COUNT reports the total active
count or only the records in the reply, so @COUNT is not parsed for detection;
switching to it stays blocked on probe evidence. The probe's comment
annotations in WnWrapAlarmConsumer.cs are preserved.

Reset semantics: not latched. WnWrapAlarmConsumer.FoldFetch replaces the
verdict on every poll under the same lock as the snapshot merge, so the first
sub-cap fetch clears it; GatewayAlarmMonitor.ClearCache drops it with the cache
generation it describes. A caveat that never turns off is one operators learn
to ignore.

Flow: WnWrapAlarmConsumer.LastSnapshotTruncated -> AlarmDispatcher (stamps every
record) / IAlarmCommandHandler (payload) -> MxAccessCommandExecutor reply ->
GatewayAlarmMonitor._snapshotTruncated -> IGatewayAlarmService.SnapshotTruncated
-> DashboardAlarmQueryResult -> AlarmsPage warning banner (render-side only; the
poll loop and DisposeAsync drain are untouched). The public QueryActiveAlarms
RPC forwards worker snapshots unmodified, so the per-record flag needed no
mapper change — a test pins that.

Parity: this describes OUR fetch mechanics — additive gateway metadata — not
MXAccess provider behavior. No event is synthesized and no MXAccess-observable
semantics change, so it is not a parity deviation.

Tests: worker LastSnapshotTruncated set/reset/consecutive-burst (windev-run);
gateway end-to-end truncated reply -> monitor -> public stream, with the
complete-reply control as the load-bearing assertion; AlarmsPage banner
present/absent. Docs: gateway.md alarm surface, docs/DesignDecisions.md entry.
2026-08-17 04:18:34 -04:00
Joseph Doherty 6c5218913b perf(dashboard): gate event mirror on live viewers — no clone, no send for unwatched sessions
DashboardEventBroadcaster.Publish ran a deep protobuf Clone (redaction is on by
default) and a group SendAsync for every event of every session, before anything
checked whether a dashboard client was actually watching. In the steady state the
session:{id} group is empty, so that work was thrown away per event.

SignalR does not expose group membership, so EventsHub now mirrors its own
add/remove into a singleton EventsHubViewerRegistry, and OnDisconnectedAsync
releases everything a dropped connection held (SessionDetailsPage disposes the
connection rather than unsubscribing). Publish returns early when the session has
no viewers, before the redaction clone. Watched sessions behave exactly as before.

Lazy mirror-lease start/stop was deliberately not attempted — it entangles the
dashboard with SessionEventDistributor subscribe lifetime for no saving beyond
this gate; recorded in docs/GatewayDashboardDesign.md.
2026-08-15 12:07:33 -04:00
Joseph Doherty e15c3cb052 fix(dashboard): redact mirror tag values when ShowTagValues off; trim value-log doc (SEC-25, SEC-30)
SEC-25 (near-term hardening; full per-session EventsHub ACL stays deferred to
roadmap item 12): DashboardEventBroadcaster now redacts tag values from a deep
CLONE of the event before mirroring to SignalR when Dashboard:ShowTagValues is
false (the default). Clears MxEvent.value (covers OnDataChange/OnWriteComplete/
OperationComplete/OnBufferedDataChange — their bodies are empty discriminators,
values ride in the top-level field incl. buffered arrays) and the alarm body's
current_value/limit_value. Source event never mutated (shared with the gRPC
path + replay ring) - verified by a source-not-mutated test. Makes the formerly
dead ShowTagValues flag live for the mirror. EventsHub TODO(per-session-acl)
kept and tied to roadmap item 12. Tests: DashboardEventBroadcasterTests (3).

SEC-30: trim docs/Diagnostics.md to mark opt-in command-value logging as NOT
YET IMPLEMENTED (no LogCommandValues knob, RedactCommandValue has no call
sites, no values logged); wiring deferred pending secured-bulk redaction
coverage (SEC-13). No option/call sites added.

Server build clean (0 warnings); Dashboard tests 152/152.

Claude-Session: https://claude.ai/code/session_01DMXXvNuPekkkrTEyPNxEkW
2026-07-09 15:28:24 -04:00
Joseph Doherty fca978de07 docs(src): add missing XML docs and strip tracking-ID comments
Sweep of 203 source files resolving CommentChecker findings: add
<summary>/<param>/<returns>/<inheritdoc> where missing, and remove
resolved task/issue tracking markers (Tests-NNN, Worker-NNN, Server-NNN,
Task N) from code comments. Comment/doc-only — no logic changes.
Server+Tests build clean under TreatWarningsAsErrors.
2026-07-07 14:09:49 -04:00
Joseph Doherty 0032d2dc44 docs+chore: fix stale prose, project names, remove dead MapSqlException (§7)
- docs/plans/2026-06-14-deferred-followups.md: mark D1 as executed
  (commit 4af24b9; metric emitted at DashboardSnapshotService.cs:198);
  note D2 resolved as no-op; D3-D5 remain pending
- docs/AlarmClientDiscovery.md §5: rewrite STA "production fix needed"
  to past tense — alarms now route through GatewayAlarmMonitor/worker STA
- EventsHub.cs: replace stale "publisher side is a future follow-up"
  comment; DashboardEventBroadcaster is live and DI-registered
- CLAUDE.md: fix all project-name drift (src/MxGateway.* →
  src/ZB.MOM.WW.MxGateway.*; MxGateway.sln → ZB.MOM.WW.MxGateway.slnx;
  clients/dotnet/MxGateway.Client.sln → ZB.MOM.WW.MxGateway.Client.slnx)
- GalaxyRepositoryGrpcService.cs: remove dead MapSqlException method and
  its IDE0051 suppression pragma; drop now-unused ILogger ctor param and
  Microsoft.Data.SqlClient using; build confirmed 0 warnings/errors
2026-06-15 09:43:00 -04:00
Joseph Doherty 615b487a77 docs+ui: backfill XML doc comments and finish dashboard layout pass
Adds missing <summary>/<param> XML docs across 99 server, worker, and test
files so CommentChecker reports zero issues (TreatWarningsAsErrors needs the
analyzer clean). Bundles in WIP dashboard work: NavSection extraction,
MainLayout/site.css/js styling alignment, and DashboardOptions/Auth tweaks.
2026-05-27 14:20:10 -04:00
Joseph Doherty 327e9c5f94 Resolve Server-031..032 (re-triaged) + Server-038..043
Server-031: re-triaged. The recommended gateway-side
"skip-while-command-in-flight" guard is already in place at
WorkerClient.HeartbeatLoopAsync via WorkerClientOptions.HeartbeatStuckCeiling
(default 75s = 5× HeartbeatGrace). Two regression tests pin the
behaviour. Recommendation #1 (decouple worker-side _writeLock) is a
Worker-module concern (Worker-017 / Worker-023) and out of scope here.

Server-032: re-triaged. Recommendation #2 (rich diagnostic) is already
in EnqueueWorkerEventAsync, with #3 (overflow grace) absorbed by the
TryWrite → WriteAsync-with-timeout fall-through. Test
EnqueueWorkerEvent_WhenChannelFullPastTimeout_FaultsWithRichDiagnostic
pins the diagnostic string. Recommendation #1 (prose contract in
gateway.md / docs) is deferred — outside this pass's edit scope.

Server-038 (Security): EventsHub.SubscribeSession's missing per-session
ACL is documented with a TODO(per-session-acl) and a <remarks> block
explaining the v1 acceptance (any dashboard role can subscribe to any
session — non-secret metadata, redacted value logging). The per-session
ACL design lands in a follow-up once a session-scoped role exists.

Server-039 (Error handling): HubTokenService.Validate now rejects a
deserialized payload where both Name and NameIdentifier are null/empty.
New test file HubTokenServiceTests.cs covers the regression and five
sanity cases. TDD confirmed.

Server-040 (Conventions): MapGroupsToRoles gains a precedence comment
explaining "full literal match first, leading-RDN fallback;
OrdinalIgnoreCase via DashboardOptions.GroupToRole". Documentation-only.

Server-041 (Design adherence): EventStreamService.ProduceEventsAsync
wraps the broadcaster.Publish call in try/catch (Exception). The
producer loop and gRPC stream are no longer at the mercy of the
broadcaster's never-throw discipline. New regression test
StreamEventsAsync_WhenDashboardBroadcasterThrows_StillYieldsEventsAndDoesNotFaultSession.

Server-042 (Performance): DashboardSnapshotPublisher.ExecuteAsync now
mirrors AlarmsHubPublisher's reconnect loop — wraps the await foreach
in a while-not-cancelled, catches general exceptions, and Task.Delays
5s before retrying. An internal ctor accepts a shorter delay for the
test. New test file DashboardSnapshotPublisherTests.cs covers the
throw-then-yield reconnect path and the normal-completion case.

Server-043 (Documentation): HubTokenService class XML doc gains a
<remarks> describing the singleton lifetime, the two consumer scopes
(DashboardHubConnectionFactory scoped, HubTokenAuthenticationHandler
transient), and the thread-safety contract.

Verification: dotnet build src/ZB.MOM.WW.MxGateway.slnx clean
(0 warnings / 0 errors); src/ZB.MOM.WW.MxGateway.Tests 486/486 passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-24 03:18:52 -04:00
Joseph Doherty 65943597d4 dashboard: side-rail layout + SignalR push hubs (snapshot, alarms, events)
Layout
------
DashboardLayout.razor replaces the inline header nav with a left side rail
modelled on the OtOpcUa admin (Dashboard B). The top bar keeps only the
brand, breadcrumb, and signed-in status pill; navigation moves into a
fixed-width 218px rail with grouped section eyebrows (Overview,
Runtime, Galaxy, Admin) and a Session footer carrying the user name,
role claims, and a Sign-out button. dashboard.css gains the
`.app-shell` flex container, `.side-rail` column, `.rail-eyebrow`,
`.rail-link[.active]`, `.rail-foot`, `.rail-user`, `.rail-roles`, and
`.rail-btn` rules (all driven by the existing theme.css tokens, no new
hard-coded colours).

SignalR (push)
--------------
Adds three hubs under `Dashboard/Hubs/`, all gated by the
`HubClientsPolicy` registered in the previous commit:

  * DashboardSnapshotHub (/hubs/snapshot)
    Broadcasts the full DashboardSnapshot on every change. Sends the
    current snapshot to a new caller in OnConnectedAsync so the first
    paint is immediate.

  * AlarmsHub (/hubs/alarms)
    Connected clients auto-join the `__alarms__` group. Receives
    AlarmFeedMessage values (active_alarm / snapshot_complete /
    transition) re-broadcast from the gateway's central alarm monitor.

  * EventsHub (/hubs/events)
    Per-session push surface. Clients call SubscribeSession(sessionId)
    to join `session:{id}`. The publisher side is intentionally a
    follow-up — the snapshot hub already carries recent-events
    rollups; a dedicated MxEvent broadcaster on EventStreamService
    will plug into this hub's group convention.

Two BackgroundService publishers wire server-side data sources to the
hubs:

  * DashboardSnapshotPublisher subscribes to
    `IDashboardSnapshotService.WatchSnapshotsAsync` and forwards every
    snapshot to all connected hub clients.
  * AlarmsHubPublisher subscribes to `IGatewayAlarmService.StreamAsync`
    (no filter) and forwards every AlarmFeedMessage to the
    `__alarms__` group, reconnecting with a 5-second backoff if the
    stream faults.

Connection + auth plumbing
--------------------------
  * `GET /hubs/token` issues a fresh data-protected bearer token
    bound to the calling user's identity and roles. Gated by the
    cookie-only ViewerPolicy so a Blazor circuit (cookie-authenticated)
    can mint a token, but a hub bearer cannot self-bootstrap a new
    one.
  * DashboardHubConnectionFactory (scoped) is the client-side helper
    Razor pages inject. It builds a HubConnection with an
    AccessTokenProvider that calls HubTokenService.Issue on every
    (re)connect — keeps the connection alive across cookie refresh
    boundaries.

Pull → push refactor
--------------------
DashboardPageBase no longer drives its own `WatchSnapshotsAsync`
async-foreach loop. It now:
  1. seeds Snapshot synchronously from `IDashboardSnapshotService.GetSnapshot()`
     so the first render is non-empty;
  2. opens a `DashboardSnapshotHub` connection via the connection
     factory;
  3. updates Snapshot + triggers StateHasChanged on each
     `SnapshotUpdated` push.

The hub connection is best-effort: if SignalR can't start, the
synchronous snapshot seed keeps the UI populated. SignalR's
WithAutomaticReconnect handles the recovery path.

Package
-------
Adds `Microsoft.AspNetCore.SignalR.Client` 10.0.0 to the server csproj
so the in-process Blazor pages can open hub connections back to their
own hosting process.

Verification: 475 server tests (+ 2 new
`DashboardHubsRegistrationTests` that pin the hub negotiate endpoints
and the singleton/scoped DI shape), 275 worker tests (+ 9 dev-rig
skips), 18 integration tests (live MxAccess + LDAP + Galaxy) all pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-24 01:48:27 -04:00