docs(plans): perf review remediation plan
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,624 @@
|
||||
# Performance Review Remediation Implementation Plan
|
||||
|
||||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:executing-plans to implement this plan task-by-task (or superpowers-extended-cc:subagent-driven-development when executing in-session).
|
||||
|
||||
**Goal:** Resolve every actionable finding from the 2026-08-15 architectural performance review — six High findings, the Medium tier, and the worthwhile Low/hygiene items — without changing any MXAccess parity behavior or public contract.
|
||||
|
||||
**Architecture:** Two phases. Phase A is gateway-side (.NET 10, builds and tests locally on macOS via `NonWindows.slnx`); Phase B is worker-side (.NET Framework 4.8 x86, which does **not** compile on this Mac — Phase B tasks are edited here and verified in one consolidated pass on the windev box via the `psbridge` skill, Task 24). No `.proto` changes anywhere in this plan, so no client regeneration is needed. All work happens on branch `perf/review-remediation`.
|
||||
|
||||
**Tech Stack:** ASP.NET Core gRPC, System.Threading.Channels, SignalR, Microsoft.Data.Sqlite, .NET Framework 4.8 STA/COM interop, protobuf (Google.Protobuf).
|
||||
|
||||
---
|
||||
|
||||
## Ground rules for every implementer (read before your task)
|
||||
|
||||
- **Build gate:** `TreatWarningsAsErrors=true`, `Nullable=enable`, analyzers at latest. New warnings fail the build — fix them, never suppress.
|
||||
- **Style:** follow `docs/style-guides/CSharpStyleGuide.md` — file-scoped namespaces, `sealed` by default, `Async` suffix, MXAccess-aligned names. Match the comment density and idiom of the file you're editing.
|
||||
- **Parity is sacred:** do not change MXAccess-visible semantics (event ordering, `OperationComplete` behavior, write-completion reply shape, per-tag ReadBulk timeout meaning). These tasks change *mechanics* (waits, locks, allocations), never observable protocol behavior, except where a task explicitly says otherwise.
|
||||
- **Never synthesize events.** Nothing in this plan may fabricate an `MxEvent`.
|
||||
- **Docs in the same commit:** when a task changes configuration, event mechanics, security behavior, or lifecycle rules, the named docs must be updated in that task's commit.
|
||||
- **Worker code (Phase B) does not compile on this machine.** `LangVersion=latest` applies, so modern syntax is fine, but only net48-era BCL APIs exist (no `Span`-taking stream overloads, no `ArgumentNullException.ThrowIfNull` — check what the file already uses). Match the existing worker idioms exactly. Verification is Task 24.
|
||||
- **Tests:** gateway tests use the FakeWorkerHarness (`src/ZB.MOM.WW.MxGateway.Tests`), no MXAccess needed. Run only your task's filter, not the full suite (full suite runs once per phase).
|
||||
- **Commit after every task**, message style: `perf(<area>): <what>` (or `fix(...)` for the two correctness bugs).
|
||||
|
||||
Verification commands used throughout:
|
||||
|
||||
```bash
|
||||
# Gateway build (macOS-safe)
|
||||
dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx
|
||||
# Targeted gateway tests
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~<TestClass>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
# Phase A — Gateway (local verification)
|
||||
|
||||
### Task 1: Named-pipe buffer sizes
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~3 min
|
||||
**Parallelizable with:** Tasks 2, 3, 4, 5, 6, 8, 10, 11, 12, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionWorkerClientFactory.cs` (`CreatePipe`, ~line 157)
|
||||
- Modify: `docs/WorkerFrameProtocol.md` (add a short "Pipe buffers" note)
|
||||
- Test: `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/` (existing factory/e2e tests must stay green; no new test — buffer size isn't observable through the .NET API)
|
||||
|
||||
**Why:** the current 5-arg `NamedPipeServerStream` overload passes `inBufferSize: 0, outBufferSize: 0`. A zero-quota byte-mode pipe forces every write to rendezvous with a pending read — lock-step IPC, and the exact failure class behind the historical windev suite wedge.
|
||||
|
||||
**Step 1: Change the overload**
|
||||
|
||||
```csharp
|
||||
private const int PipeBufferSizeBytes = 128 * 1024;
|
||||
|
||||
private static NamedPipeServerStream CreatePipe(string pipeName)
|
||||
{
|
||||
return new NamedPipeServerStream(
|
||||
pipeName,
|
||||
PipeDirection.InOut,
|
||||
maxNumberOfServerInstances: 1,
|
||||
PipeTransmissionMode.Byte,
|
||||
PipeOptions.Asynchronous,
|
||||
inBufferSize: PipeBufferSizeBytes,
|
||||
outBufferSize: PipeBufferSizeBytes);
|
||||
}
|
||||
```
|
||||
|
||||
Add a comment stating *why* (zero-quota rendezvous behavior; reference the windev wedge). Note: on Unix these sizes are advisory (Unix domain socket), which is fine — the fix targets Windows production.
|
||||
|
||||
**Step 2:** `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx` → 0 errors.
|
||||
**Step 3:** `dotnet test ... --filter "FullyQualifiedName~GatewayEndToEndFakeWorkerSmokeTests"` → PASS.
|
||||
**Step 4:** Update `docs/WorkerFrameProtocol.md` with a 3–4 line "Pipe buffers" paragraph. Commit: `perf(ipc): give worker pipes real OS buffers instead of zero-quota rendezvous`
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Metrics — pull-gauge for worker queue depth, lock-free command counters
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 1, 3, 4, 5, 6, 8, 10, 11, 12, 13, 14 (NOT Task 7 — both edit `WorkerClient.cs`)
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs` (`SetWorkerEventQueueDepth` ~290; `CommandStarted/Succeeded/Failed` ~202–247; gauge wiring ~91; snapshot ~461–492)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs` (call sites ~303 and ~602)
|
||||
- Test: `src/ZB.MOM.WW.MxGateway.Tests/Metrics/` (extend the existing GatewayMetrics test class)
|
||||
|
||||
**Why:** `SetWorkerEventQueueDepth` takes the process-wide `_syncRoot` twice per event for every session, and the single scalar makes the gauge last-writer-wins across sessions (a correctness bug). The command counters take the same global lock 2–3× per RPC.
|
||||
|
||||
**Step 1 (failing test):** add a test that registers two worker-queue-depth sources reporting 3 and 4 and asserts the snapshot/gauge reports 7; add a test that `CommandStarted`×N from parallel tasks yields exactly N with no lock (behavioral: just correctness of count).
|
||||
|
||||
**Step 2 (implement):**
|
||||
- Mirror the existing GWC-15 pattern verbatim: add `RegisterWorkerEventQueueDepthSource(Func<int> depth)` returning an `IDisposable` handle, a `ConcurrentDictionary<long, Func<int>>` of sources, and make `GetWorkerEventQueueDepth` sum the sources (clamp negatives). Delete `SetWorkerEventQueueDepth` and the `_workerEventQueueDepth` field.
|
||||
- `WorkerClient`: at construction (or first use), register a source returning its staged+channel depth via `Volatile.Read` of a field the stage/consume paths maintain with `Interlocked` — the hot path does **no** metrics call at all anymore. Dispose the registration in `DisposeAsync`.
|
||||
- Command counters: `_commandsStarted/_commandsSucceeded/_commandsFailed` become `long` updated with `Interlocked.Increment`; `_commandFailuresByMethod` becomes `ConcurrentDictionary<string, long>` (follow the existing `EventReceived` pattern in the same file). Snapshot reads with `Interlocked.Read`.
|
||||
|
||||
**Step 3:** run the Metrics test filter → PASS. **Step 4:** grep the repo for `SetWorkerEventQueueDepth` — zero hits outside tests you updated.
|
||||
**Step 5:** Commit: `perf(metrics): pull-model worker queue gauge (fixes last-writer-wins), Interlocked command counters`
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Distributor — copy-on-write subscriber snapshot
|
||||
|
||||
**Classification:** high-risk (core event fan-out concurrency)
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 1, 2, 4, 5, 6, 8, 10, 11, 12, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs` (pump loop ~600; register/unregister paths; the "snapshot-free enumerator" remark ~71)
|
||||
- Test: `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/` (existing SessionEventDistributor tests must stay green; add one test if a register-during-pump race test doesn't already exist)
|
||||
|
||||
**Why:** `_subscribers.Values` (the property) locks the whole `ConcurrentDictionary` and materializes a snapshot list **per event**, contradicting the adjacent comment.
|
||||
|
||||
**Step 1 (implement):** maintain a `volatile Subscriber[] _subscriberSnapshot` rebuilt inside the existing registration lock on every register/unregister (the set is tiny and mutates rarely). The pump iterates the array. Keep the dictionary if other paths use keyed lookup; the array is purely the fan-out view. Update the ~71 remark to describe the actual mechanism. Semantics to preserve exactly: a subscriber registered mid-iteration may miss the in-flight event ("late subscribers see events after they register") — the array snapshot preserves this naturally.
|
||||
|
||||
**Step 2:** run the distributor/replay test filters (`FullyQualifiedName~SessionEventDistributor`, `~Replay`) → PASS. The replay-handoff atomicity tests are the critical gate here.
|
||||
**Step 3:** Commit: `perf(events): copy-on-write subscriber snapshot in fan-out pump`
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Dashboard event mirror — viewer gating
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 1, 2, 3, 5, 6, 8, 10, 11, 12, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHub.cs`
|
||||
- Create: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHubViewerRegistry.cs`
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs` (Publish, ~39)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs` (register the registry)
|
||||
- Modify: `docs/GatewayDashboardDesign.md` (mirror gating paragraph)
|
||||
- Test: create `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/EventsHubViewerRegistryTests.cs` + extend the existing DashboardEventBroadcaster tests
|
||||
|
||||
**Why:** with `ShowTagValues=false` (default), `Publish` deep-clones every event and dispatches to a SignalR group that is empty in the steady state. No viewer gate exists anywhere on the path.
|
||||
|
||||
**Step 1 (failing test):** broadcaster with zero registered viewers for the session performs **no clone and no send** (assert via a counting fake hub-clients/`IHubContext` seam, matching however the existing broadcaster tests fake SignalR); with one viewer, behavior is unchanged (redacted clone sent).
|
||||
|
||||
**Step 2 (implement):**
|
||||
- `EventsHubViewerRegistry` (singleton): `ConcurrentDictionary<string, int>` session→viewer count, `Increment(sessionId)`, `Decrement(sessionId)`, `HasViewers(sessionId)`. Track per-connection subscribed sessions in a `ConcurrentDictionary<string, ConcurrentDictionary<string,byte>>` keyed by connection id so `OnDisconnectedAsync` can decrement everything that connection held.
|
||||
- `EventsHub`: `SubscribeSession`/`UnsubscribeSession` update the registry alongside the group add/remove; override `OnDisconnectedAsync` to release the connection's sessions. Keep the existing SEC-25 remark intact.
|
||||
- `DashboardEventBroadcaster.Publish`: first line after the null-guards becomes `if (!viewerRegistry.HasViewers(sessionId)) { return; }` — before the redact/clone.
|
||||
- Do **not** attempt lazy mirror-lease start in this task (it interacts with distributor lifecycle); the gate above removes ~all of the waste already. Note this decision in the doc paragraph.
|
||||
|
||||
**Step 3:** run Dashboard test filter → PASS. **Step 4:** update `docs/GatewayDashboardDesign.md`. Commit: `perf(dashboard): gate event mirror on live viewers — no clone, no send for unwatched sessions`
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Snapshot pipeline — idle gating, cached config, keyed refresh cadence
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 1, 2, 3, 4, 6, 8, 10, 11, 12, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotPublisher.cs` (~69–83)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotHub.cs` (connection counting)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs` (~103 config rebuild, ~163–164 + ~267 API-key refresh)
|
||||
- Modify: `docs/GatewayDashboardDesign.md`
|
||||
- Test: extend existing snapshot service/publisher tests under `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/`
|
||||
|
||||
**Why:** the 1 Hz tick runs an API-key SQLite read, a registry sort, a metrics snapshot, and a rebuild of the *static* effective-configuration record, broadcast to `Clients.All`, forever, with zero viewers.
|
||||
|
||||
**Step 1 (failing tests):** (a) effective configuration object is reference-identical across two snapshot builds; (b) API-key summaries refresh at most once per configured interval (inject `TimeProvider`, follow the file's existing time idiom); (c) publisher with zero connections does not enumerate the snapshot source (fake the hub context; count pulls).
|
||||
|
||||
**Step 2 (implement):**
|
||||
- Cache `EffectiveGatewayConfiguration` in a field on first build (it's startup-static; add a comment saying so).
|
||||
- `RefreshApiKeySummariesAsync`: skip unless `RefreshInterval` (new private constant, 15 s) has elapsed since the last successful refresh.
|
||||
- `DashboardSnapshotHub`: `OnConnectedAsync`/`OnDisconnectedAsync` maintain an `int` connection count on a small singleton (or reuse the Task 4 registry class with a well-known key — implementer's choice, keep it simple). Publisher checks the count each tick: zero connections → `await Task.Delay(interval)` and skip both the snapshot build and the broadcast. First connection after idle gets a fresh snapshot on its next tick (≤1 interval of staleness — acceptable; pages also seed from `IDashboardSnapshotService` directly on load).
|
||||
|
||||
**Step 3:** dashboard test filter → PASS. Docs paragraph. Commit: `perf(dashboard): idle-gate the snapshot tick; cache static config; bound key-list refresh`
|
||||
|
||||
---
|
||||
|
||||
### Task 6: Reply ownership transfer in `MapCommandReply`
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~3 min
|
||||
**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 8, 10, 11, 12, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGrpcMapper.cs` (~74)
|
||||
- Test: existing mapper/service tests under `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/`
|
||||
|
||||
**Why:** every `WorkerCommandReply` is parsed fresh from one pipe frame and completed to exactly one awaiter; the gRPC handler is its only consumer. Events already got this treatment under GWC-07 — replies still deep-copy, which doubles the largest hot-path message on bulk reads.
|
||||
|
||||
**Step 1 (verify precondition, in-code):** confirm (grep) that no caller of `WorkerClient.InvokeAsync` retains `reply.Reply` after mapping — the review found the Invoke path clean; `GatewayAlarmMonitor` and `DashboardLiveDataService` own their separate replies. If you find a second consumer, STOP and surface it — that's a plan defect.
|
||||
|
||||
**Step 2 (implement):** `return reply.Reply.Clone();` → `return reply.Reply;` with a GWC-07-style ownership comment: the worker reply object is single-consumer by construction (one frame → one `PendingCommand` completion → one mapper call); the mapper transfers ownership to the gRPC response.
|
||||
|
||||
**Step 3:** run `FullyQualifiedName~MxAccessGrpcMapper` + the fake-worker smoke filter → PASS. Commit: `perf(grpc): transfer reply ownership instead of deep-cloning every worker reply`
|
||||
|
||||
---
|
||||
|
||||
### Task 7: WorkerClient — pooled-timer timeout, single sizing pass, `WorkerCancel` on timeout
|
||||
|
||||
**Classification:** high-risk (IPC concurrency + protocol behavior)
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 3, 4, 5, 8, 10, 11, 12, 13, 14 (NOT Task 2 — both edit `WorkerClient.cs`; run after Task 2)
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs` (InvokeAsync ~226–270; timeout path)
|
||||
- Modify: `docs/GatewayProcessDesign.md` (command timeout → cancel-forwarding note)
|
||||
- Test: extend `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/` worker-client tests (the fakes there already exercise timeout paths)
|
||||
|
||||
**Why:** each Invoke churns a linked CTS + `Task.Delay` timer + `WhenAny`; `CalculateSize` runs twice (protobuf doesn't memoize); and on timeout the gateway never tells the worker, so a timed-out COM call keeps occupying the STA and an envelope still queued gets written anyway.
|
||||
|
||||
**Step 1 (failing test):** on command timeout, the client enqueues a `WorkerCancel` envelope carrying the timed-out correlation id (assert via the fake connection's written-frame log).
|
||||
|
||||
**Step 2 (implement):**
|
||||
- Replace the CTS/Delay/WhenAny block with `await pendingCommand.Task.WaitAsync(timeout, cancellationToken)` wrapped in a `try/catch (TimeoutException)` / `(OperationCanceledException)` mapping to the exact same `WorkerClientErrorCode`s and messages as today (tests depend on them).
|
||||
- On the timeout path, after `RemovePendingCommandAsFailed`, best-effort enqueue a `WorkerCancel` envelope for the correlation id (fire-and-forget with a swallow-and-log; never let cancel failure mask the timeout exception). The worker already handles `WorkerCancel` (`WorkerPipeSession` → `CancelCommand`).
|
||||
- Thread the already-computed `envelopeSize` into the frame write path if the writer API allows passing a known size; if the writer's public surface would have to change more than trivially, skip this sub-item and leave a `// PERF:` note — the timer and cancel fixes carry the task.
|
||||
|
||||
**Step 3:** worker-client test filter → PASS, including existing timeout tests unchanged. Docs note. Commit: `perf(ipc): WaitAsync command timeouts + forward WorkerCancel so a timed-out COM call frees the STA`
|
||||
|
||||
---
|
||||
|
||||
### Task 8: Audit pipeline — startup bootstrap, background writer, retention
|
||||
|
||||
**Classification:** high-risk (security/audit semantics)
|
||||
**Estimated implement time:** ~5 min (split if it runs long: 8a writer, 8b retention)
|
||||
**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 6, 7, 10, 11, 12, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Audit/SqliteCanonicalAuditStore.cs` (per-op `EnsureTableAsync` ~52–54, ~94, ~131–136)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Audit/CanonicalAuditWriter.cs` (~35)
|
||||
- Create: `src/ZB.MOM.WW.MxGateway.Server/Security/Audit/ChannelAuditWriter.cs` (bounded channel + hosted drain)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/AuthStoreServiceCollectionExtensions.cs` (DI wiring + hosted service)
|
||||
- Modify: `docs/DesignDecisions.md` (audit is asynchronous best-effort, bounded, with retention)
|
||||
- Test: create `src/ZB.MOM.WW.MxGateway.Tests/Security/Audit/ChannelAuditWriterTests.cs`
|
||||
|
||||
**Why:** constraint denials await a SQLite insert inline per denied tag inside bulk RPC loops — sequential round-trips into the same DB file the auth store uses, each with a redundant `CREATE TABLE IF NOT EXISTS`, into a table with no retention.
|
||||
|
||||
**Step 1 (failing tests):** (a) `WriteAsync` returns without touching the store (enqueue-only) and the event lands in the store shortly after (drain); (b) when the bounded channel (capacity 4096) is full, `WriteAsync` drops (oldest or newest — pick drop-write/newest for simplicity) and increments a counter, never blocks; (c) retention sweep deletes rows older than the configured window.
|
||||
|
||||
**Step 2 (implement):**
|
||||
- `ChannelAuditWriter : ICanonicalAuditWriter` (or whatever the current writer interface is named — read `CanonicalAuditWriter.cs` first): bounded `Channel<CanonicalAuditEvent>` (`BoundedChannelFullMode.DropWrite`), a `BackgroundService` drain that batches up to 64 events into one transaction per drain pass. The audit contract is already documented best-effort — say so in the class doc.
|
||||
- Table bootstrap: run `EnsureTableAsync` once from the drain service's `StartAsync` (and from the store's first list call via a `Lazy`/latch); remove the per-insert and per-list calls.
|
||||
- Retention: in the same drain service, once per hour, `DELETE FROM audit_event WHERE timestamp < now - RetentionDays` (new `SecurityOptions`/audit option, default 90 days, validated ≥1 in `GatewayOptionsValidator`); document in `docs/GatewayConfiguration.md`.
|
||||
- Wire DI so `ConstraintEnforcer.RecordDenialAsync` transparently goes through the channel writer — **no signature changes** at the enforcer/service layer.
|
||||
- Flush-on-shutdown: drain the channel in `StopAsync` with a 2 s cap.
|
||||
|
||||
**Step 3:** audit test filter + `FullyQualifiedName~ConstraintEnforcer` → PASS. Docs (`DesignDecisions.md`, `GatewayConfiguration.md`). Commit: `perf(audit): bounded async audit writer with batched inserts, one-time bootstrap, retention sweep`
|
||||
|
||||
---
|
||||
|
||||
### Task 9: Parallel session teardown in sweep and shutdown
|
||||
|
||||
**Classification:** high-risk (lifecycle concurrency)
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Tasks 10, 11, 12, 13, 14 (edits only `SessionManager.cs` + docs; run any time)
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs` (`CloseExpiredLeasesAsync` ~256–296, `ShutdownAsync` ~301–329)
|
||||
- Modify: `docs/Sessions.md` (teardown parallelism note)
|
||||
- Test: extend `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/` session-manager tests
|
||||
|
||||
**Why:** both loops `await CloseSessionCoreAsync` strictly sequentially, each bounded by the 10 s worker-shutdown timeout — a mass expiry with hung workers stalls slot reclamation, and 50-session shutdown exceeds any host stop-timeout.
|
||||
|
||||
**Step 1 (failing test):** two sessions whose fake worker shutdowns each take T complete a sweep in ~T, not ~2T (the fake harness supports delayed shutdown; if not, add a delay knob to the fake).
|
||||
|
||||
**Step 2 (implement):** wrap both loops in `Parallel.ForEachAsync` with `MaxDegreeOfParallelism = 4` (named constant, comment why: bounded so a mass expiry can't stampede worker teardown). `TryBeginCloseIfExpired` already makes per-session close idempotent/exclusive — state that in a comment; that's the invariant making this safe. Preserve the existing sweep precedence (lease-expiry → faulted → detach-grace) by keeping the *selection* phase sequential and parallelizing only the close calls on the selected set.
|
||||
|
||||
**Step 3:** session-manager filter → PASS. Docs. Commit: `perf(sessions): bounded-parallel teardown in lease sweep and shutdown`
|
||||
|
||||
---
|
||||
|
||||
### Task 10: Dashboard live-data subscription cap
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Tasks 1–9, 11, 12, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardLiveDataService.cs` (~61–70, `_subscribed`)
|
||||
- Modify: `docs/GatewayDashboardDesign.md`
|
||||
- Test: extend existing live-data tests under `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/`
|
||||
|
||||
**Why:** every tag any viewer ever inspected stays advised on the shared worker session forever.
|
||||
|
||||
**Step 1 (failing test):** subscribing tag #257 when the cap is 256 unsubscribes the least-recently-read tag first (assert the fake session sees an `UnsubscribeBulk`/equivalent for the evicted tag).
|
||||
|
||||
**Step 2 (implement):** replace `_subscribed` (set) with an LRU: `Dictionary<string, LinkedListNode<string>>` + `LinkedList<string>` under the existing `_gate` (already serialized — no new locking). Cap at 256 (named constant; comment the sizing rationale: one browse page of tags plus headroom). On read of an already-subscribed tag, move to front. On insert past cap, evict from the back and call the session's unsubscribe for the evicted batch. On `InvalidateSession`, clear both structures (existing behavior).
|
||||
|
||||
**Step 3:** dashboard filter → PASS. Docs. Commit: `perf(dashboard): LRU cap on the shared live-read session's advised set`
|
||||
|
||||
---
|
||||
|
||||
### Task 11: Alarm monitor — cached `CurrentAlarms` projection
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~3 min
|
||||
**Parallelizable with:** Tasks 1–10, 12, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs` (~90–99 + every mutation site under `_sync`)
|
||||
- Test: extend `src/ZB.MOM.WW.MxGateway.Tests/Alarms/` monitor tests
|
||||
|
||||
**Why:** `CurrentAlarms` clones the full alarm set under the broadcast lock on every call.
|
||||
|
||||
**Step 1 (failing test):** two consecutive `CurrentAlarms` calls with no intervening transition return the same cached array instance; a transition invalidates it.
|
||||
|
||||
**Step 2 (implement):** add `private IReadOnlyList<ActiveAlarmSnapshot>? _currentAlarmsCache;` — `CurrentAlarms` builds it (still cloning, still under `_sync`) only when null; every mutation path that touches the alarm dictionary (`ApplyTransition`, reconcile apply, clear) nulls it under `_sync`. Callers already treat the result as read-only.
|
||||
|
||||
**Step 3:** alarms filter → PASS. Commit: `perf(alarms): memoize CurrentAlarms projection, invalidate on mutation`
|
||||
|
||||
---
|
||||
|
||||
### Task 12: Request-logging middleware — hoisted logger, bearer redaction fix
|
||||
|
||||
**Classification:** small (contains a security fix)
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Tasks 1–11, 13, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayRequestLoggingMiddlewareExtensions.cs` (~29–38)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs` (~54–77)
|
||||
- Test: extend `src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/` redactor tests
|
||||
|
||||
**Why:** `CreateLogger` (factory lock + DI resolve) per request; and — the security half — `RedactClientIdentity` passes any bearer credential that doesn't contain `mxgw_` through **unredacted** into log scope, violating the "never log secrets" convention.
|
||||
|
||||
**Step 1 (failing test):** `RedactClientIdentity("Bearer eyJhbGciOi...")` (a non-mxgw token) returns a redacted form (e.g. `Bearer [redacted]`), never the raw token. Keep the existing mxgw-shaped redaction (`mxgw_<id>_***`) intact — those tests must still pass.
|
||||
|
||||
**Step 2 (implement):**
|
||||
- Redactor: any `authorization`-style value that is not recognized as an mxgw key redacts to a fixed `"[redacted]"` (preserve scheme word only). This is fail-closed.
|
||||
- Middleware: resolve the `ILogger` once outside the per-request lambda (category-keyed, not request-keyed) via the app's `ILoggerFactory` at `Use...` registration time; keep the scope construction as-is (it carries per-request fields the log pipeline consumes — do not conditionalize it on log level in this task; note as considered-and-skipped since scope consumers may be added at runtime).
|
||||
|
||||
**Step 3:** diagnostics filter → PASS. Commit: `fix(logging): fail-closed bearer redaction; hoist per-request logger creation`
|
||||
|
||||
---
|
||||
|
||||
### Task 13: Auth-path hygiene — span token parse, limiter partition keys
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Tasks 1–12, 14
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs` (~153 `TryResolveKeyId`)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/CachingApiKeyVerifier.cs` (~229 `TryParseKeyId`)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs` (~244–269, ~398)
|
||||
- Test: existing auth tests under `src/ZB.MOM.WW.MxGateway.Tests/Security/` must stay green; add parse-equivalence cases
|
||||
|
||||
**Step 1 (failing test):** parse-equivalence table test: for a set of tokens (well-formed, missing `_`, empty, extra `_`), the new span parser returns exactly what `Split('_')` logic returned.
|
||||
|
||||
**Step 2 (implement):** replace `Split('_')` in both parsers with `IndexOf('_')` twice over a `ReadOnlySpan<char>`/string (no arrays, no substrings until the final key-id slice). In the limiter, compute the composite partition key once per RPC and pass it to both `Check` and `Reset` (or add an overload taking the precomputed key) instead of concatenating twice.
|
||||
|
||||
**Step 3:** security filter → PASS. Commit: `perf(auth): allocation-free token parsing; single partition-key build per RPC`
|
||||
|
||||
---
|
||||
|
||||
### Task 14: Bulk constraint loops, caches, and per-call hygiene
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 1–13 (NOT Task 6 if the mapper edit collides — it doesn't; different files)
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs` (bulk loops ~466–troughs at 494/551/612/680; double session resolve ~104/126)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs` (~214–215 LINQ; expose `HasReadConstraints`/`HasWriteConstraints` if not present)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/GatewayApiKeyIdentityMapper.cs` (~39–42 cache cliff)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Sessions/SparseArrayExpander.cs` (~123–126 capacity hints)
|
||||
- Test: existing constraint/service tests under `src/ZB.MOM.WW.MxGateway.Tests/` + one new eviction test
|
||||
|
||||
**Step 1 (failing test):** constraint-blob cache: inserting entry `MaxCachedConstraintBlobs + 1` evicts the oldest instead of refusing to cache (FIFO like `GalaxyGlobMatcher` — copy its idiom).
|
||||
|
||||
**Step 2 (implement):**
|
||||
- Bulk loops: hoist a single `identity has no read/write constraints` check before each per-item loop → unconstrained keys take an O(1) fast path (no per-item async interface dispatch, no denial bookkeeping allocation).
|
||||
- Glob matching: replace the two `.Any(lambda)` calls with `for` loops over the glob lists.
|
||||
- Denied-path double clone: build the filtered command directly (new message, copy allowed entries in) instead of `command.Clone()` then clear-and-refill; `MapCommand`'s own clone stays (that one is the load-bearing no-aliasing copy).
|
||||
- Session double-resolve: add/`use` a `SessionManager` overload accepting the already-resolved `GatewaySession` (or have the service pass the session it resolved); keep the not-found exception behavior identical.
|
||||
- `SparseArrayExpander`: set `RepeatedField.Capacity = length` (per element type) before the fill loops.
|
||||
|
||||
**Step 3:** run `FullyQualifiedName~ConstraintEnforcer`, `~MxAccessGatewayService`, `~SparseArray` filters → PASS. Commit: `perf(grpc): O(1) unconstrained bulk fast path, direct filtered-command build, cache eviction, capacity hints`
|
||||
|
||||
---
|
||||
|
||||
### Task 15: Phase A gate — full gateway suite
|
||||
|
||||
**Classification:** trivial (verification only)
|
||||
**Estimated implement time:** ~5 min wall (suite runtime)
|
||||
**Parallelizable with:** none (runs after Tasks 1–14)
|
||||
|
||||
Run, in order:
|
||||
|
||||
```bash
|
||||
dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj
|
||||
```
|
||||
|
||||
Expected: 0 build errors, full suite green, clean process exit (0 surviving testhost). Fix anything red before Phase B. Commit only if fixes were needed.
|
||||
|
||||
---
|
||||
|
||||
# Phase B — Worker (.NET Framework 4.8; verified on windev in Task 24)
|
||||
|
||||
> Phase B implementers: you cannot compile. Be conservative — minimal diffs, match file idioms, net48 BCL only. Every task here lands as an unverified commit that Task 24 builds and tests remotely; keep commits clean so a failure bisects trivially.
|
||||
|
||||
### Task 16: Event drain loop — wake signal instead of 25 ms poll
|
||||
|
||||
**Classification:** high-risk (event path liveness)
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 18, 19, 21, 22, 23
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs` (add wake handle; `Enqueue` sets it)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs` (~18 `EventDrainInterval`, ~345–372 drain loop)
|
||||
- Modify: `docs/MxAccessWorkerInstanceDesign.md` (drain-loop paragraph ~381)
|
||||
- Test: extend `src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/` event-queue tests + `Ipc/` pipe-session tests (they run on windev)
|
||||
|
||||
**Why:** the drain loop polls at 25 ms with no wake from `Enqueue` — a 25 ms latency floor on every burst from idle, 40 wakeups/s per idle worker, and less burst absorption before the 10k queue faults the session.
|
||||
|
||||
**Implement:**
|
||||
- `MxAccessEventQueue`: add a `SemaphoreSlim _signal = new(0, 1)` (or an `AsyncAutoResetEvent`-shaped helper if the codebase has one — check first). `Enqueue` releases it (cap at 1, swallow `SemaphoreFullException`). Expose `Task WaitForEventsAsync(TimeSpan timeout, CancellationToken ct)`.
|
||||
- Drain loop: when a drain returns empty, `await queue.WaitForEventsAsync(EventDrainInterval, ct)` instead of `Task.Delay` — the 25 ms becomes a *fallback* ceiling, not the floor; a signaled wait returns immediately. Loop structure otherwise unchanged (fault handling, batch size).
|
||||
- Doc paragraph: drain is signal-driven with a 25 ms fallback tick.
|
||||
- Tests: enqueue-after-idle results in a drain without waiting for the fallback interval (windev-run; write it now).
|
||||
|
||||
Commit: `perf(worker): signal-driven event drain — removes the 25 ms latency floor and idle wakeups`
|
||||
|
||||
---
|
||||
|
||||
### Task 17: Event queue capacity — launcher-configurable
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 18, 19, 21, 22, 23 (NOT Task 16 — both edit `MxAccessEventQueue.cs`/`WorkerPipeSession.cs`; run after 16)
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Configuration/WorkerOptions.cs` (+`EventQueueCapacity`, default 10000) and `GatewayOptionsValidator.cs` (≥1000, ≤1_000_000)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerProcessLauncher.cs` (new env var, mirror the `WorkerWriteCompletionWaitEnvironmentVariableName` pattern at ~25–29 and ~186–187 exactly)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Bootstrap/WorkerOptionsParser.cs` / `WorkerOptions.cs` / `EnvironmentVariableWorkerEnvironment.cs` (read it, following the write-completion variable's path)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessStaSession.cs` (~52: pass capacity to `new MxAccessEventQueue(...)`)
|
||||
- Modify: `docs/GatewayConfiguration.md` (+`MxGateway:Worker:EventQueueCapacity`), `docs/MxAccessWorkerInstanceDesign.md` (capacity paragraph ~371)
|
||||
- Test: gateway side — validator test + launcher env-var test (these run locally); worker side — parser test (windev)
|
||||
|
||||
**Why:** the 10,000 default is headroom-critical (overflow faults the session) but not configurable without a rebuild.
|
||||
|
||||
**Implement:** copy the `WriteCompletionWaitMilliseconds` plumbing end to end under a new name (`MXGW_EVENT_QUEUE_CAPACITY` shaped like the existing variable's naming). Absent/invalid env value → default 10000 (never crash the worker on a bad value; log and default).
|
||||
|
||||
Note the gateway-side files here don't overlap Phase A tasks — safe after Task 15.
|
||||
|
||||
Commit: `perf(worker): launcher-configurable event queue capacity`
|
||||
|
||||
---
|
||||
|
||||
### Task 18: STA completion waits — message-driven, not sleep-polled
|
||||
|
||||
**Classification:** high-risk (STA/pump semantics)
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 16, 17, 19, 21, 22, 23
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessWriteCompletionCache.cs` (~97–118 wait loop)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessValueCache.cs` (~135–150 wait loop)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Sta/StaMessagePump.cs` (if it doesn't already expose a bounded "pump until signaled or timeout" primitive)
|
||||
- Modify: `docs/MxAccessWorkerInstanceDesign.md`
|
||||
- Test: extend `src/ZB.MOM.WW.MxGateway.Worker.Tests/Sta/` + `MxAccess/` cache tests (windev)
|
||||
|
||||
**Why:** both waits run `pumpStep(); ...; Thread.Sleep(5)` on the STA — during each 5 ms sleep no messages pump, so COM event dispatch stalls in 5 ms bites for up to 1.5 s (writes) / 1 s per tag (ReadBulk).
|
||||
|
||||
**Implement:**
|
||||
- Add a wake to both caches: the update path (`OnWriteComplete` recording a completion / `OnDataChange` recording a value) signals a Win32 auto-reset event (`AutoResetEvent` is fine — it wraps one).
|
||||
- Replace `Thread.Sleep(pollIntervalMs)` with a pump-integrated wait: `MsgWaitForMultipleObjectsEx(1, [waitHandle], remainingMs-capped-at-50, QS_ALLINPUT, MWMO_INPUTAVAILABLE)`; on `WAIT_OBJECT_0 + 1` (message arrived) run `pumpStep()` and re-check; on `WAIT_OBJECT_0` (signaled) re-check the entry immediately. The existing `StaMessagePump`/`StaRuntime` already use exactly this Win32 pattern (~`StaRuntime.cs:255–261`) — reuse/extract their P/Invoke declarations, do not duplicate.
|
||||
- **Semantics unchanged:** timeouts, deadline math, return values, and the unconfirmed-empty-statuses reply shape stay byte-identical. Only the *waiting mechanism* changes: latency to observe a completion drops from ≤5 ms granularity to immediate, and the pump keeps running throughout the wait.
|
||||
- **Do not** change the plain-`Write` completion-wait default in this task. The 1.5 s default is a documented OtOpcUa contract (`MxGateway:Worker:WriteCompletionWaitMilliseconds` is already configurable). Leave a doc note that operators with pure fire-and-forget write workloads can lower it.
|
||||
|
||||
Commit: `perf(worker): message-driven completion waits — the STA pumps continuously while waiting`
|
||||
|
||||
---
|
||||
|
||||
### Task 19: Handle registry — reverse index, cached views, O(1) removals
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 16, 17, 18, 21, 22, 23
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessHandleRegistry.cs` (`ItemHandles`/`ServerHandles`/`AdviceHandles` properties ~14–26; `RemoveAdviceHandles` ~137–148; `UnregisterServerHandle` ~46–65)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessSession.cs` (`TryGetCachedReadFor` ~988–1000)
|
||||
- Test: extend `src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/` registry tests (windev)
|
||||
|
||||
**Why:** the sorted list properties re-sort and copy the whole table on **every access**, `TryGetCachedReadFor` reads `ItemHandles` once per ReadBulk tag (O(tags × items·log items)), and advice/server removals do full LINQ scans (O(n²) bulk teardown).
|
||||
|
||||
**Implement:**
|
||||
- Reverse index: `Dictionary<long, Dictionary<string, int>>` server→(tagAddress→itemHandle) — or flat `Dictionary<(int,int-packed + tag)>` — maintained on register/unregister. `TryGetCachedReadFor` becomes two dictionary probes (the file's own comment already asks for this).
|
||||
- Cached materialization: memoize each sorted array with a version stamp bumped on any mutation; property returns the cached array when the version matches. Registry is STA-confined (verify: no locking in the file today ⇒ single-threaded by contract — state it in a comment), so no locking needed.
|
||||
- Removals: secondary index advice-by-item (`Dictionary<long, List<advice>>` keyed on the packed `(serverHandle, itemHandle)` the item table already uses) so `RemoveAdviceHandles`/`UnregisterServerHandle` stop scanning.
|
||||
|
||||
Commit: `perf(worker): reverse tag index + memoized views + indexed removals in the handle registry`
|
||||
|
||||
---
|
||||
|
||||
### Task 20: Event conversion — exact-format timestamps, compiled status accessors
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 16, 17, 18, 19, 21, 22, 23 (different files)
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventMapper.cs` (~360–377 timestamp parse)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Conversion/MxStatusProxyConverter.cs` (~96–109 reflection reads)
|
||||
- Test: extend `src/ZB.MOM.WW.MxGateway.Worker.Tests/Conversion/` (windev) — these files have solid existing tests; add exact-format cases
|
||||
|
||||
**Implement:**
|
||||
- Timestamps: try `DateTime.TryParseExact` against a small cached array of the observed MXAccess formats (`M/d/yyyy h:mm:ss.fff tt` and its zero-padded/24 h siblings — derive the list from the existing tests' fixture strings) **first**, falling back to the existing two-stage `TryParse` chain so behavior never regresses on an unexpected locale. Order: exact formats → current-culture → invariant (today's chain).
|
||||
- Status fields: replace the per-read `field.GetValue` with delegates compiled once per field via `Expression.Lambda<Func<object, T>>` (net48-safe) cached alongside the existing `FieldInfo` cache. Same values out, no boxing per event.
|
||||
|
||||
Commit: `perf(worker): exact-format timestamp parse and compiled status-field accessors on the event path`
|
||||
|
||||
---
|
||||
|
||||
### Task 21: Event queue drain — size memoized at enqueue
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Tasks 18, 19, 20, 22, 23 (NOT 16/17 — same file; run after them)
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs` (~249–269 byte-budgeted `Drain`; enqueue path ~152–170)
|
||||
- Test: extend the windev event-queue tests: budget math unchanged for a mixed-size batch
|
||||
|
||||
**Why:** `Drain(maxEvents, maxTotalBytes)` calls `CalculateSize()` per event **inside** the queue lock the STA needs to enqueue — a large drain stalls COM callbacks.
|
||||
|
||||
**Implement:** compute `CalculateSize()` once at enqueue time (outside any lock — the caller owns the event exclusively there) and store it on the queue's node/wrapper alongside the event; `Drain` uses the memoized size. The WRK-21 never-strand-the-head guarantee is untouched (same comparisons, precomputed operand). Events are never mutated after enqueue (WRK-11 no-clone contract) so the memoized size cannot go stale — say so in a comment.
|
||||
|
||||
Commit: `perf(worker): memoize event frame size at enqueue; drain stops sizing under the STA's lock`
|
||||
|
||||
---
|
||||
|
||||
### Task 22: Worker frame writer/reader — pooled buffers
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Tasks 16, 17, 18, 19, 20, 21, 23
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs` (~467 per-frame `new byte[]`)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs` (~33 per-frame prefix buffer)
|
||||
- Test: windev `Ipc/` frame tests must stay green (they're thorough — rely on them)
|
||||
|
||||
**Why:** the worker side allocates a fresh frame buffer + prefix buffer per frame while the gateway side already pools (`ArrayPool`, GWC-30) — the fix was applied on one side only. `System.Buffers` is already referenced by the worker (its reader uses `ArrayPool.Shared`).
|
||||
|
||||
**Implement:** mirror the gateway codec: rent the frame buffer from `ArrayPool<byte>.Shared`, write prefix+payload into it, return in a `finally`; hoist the 4-byte prefix buffer to an instance field on the reader (single-reader by contract — copy the gateway reader's comment). Exact same wire bytes.
|
||||
|
||||
Commit: `perf(worker): pooled frame buffers — brings the net48 codec up to the gateway side's GWC-30 pattern`
|
||||
|
||||
---
|
||||
|
||||
### Task 23: Alarm consumer — cheap parse, truncation detection, configurable cadence
|
||||
|
||||
**Classification:** high-risk (alarm correctness)
|
||||
**Estimated implement time:** ~5 min (split 23a parse / 23b truncation+config if long)
|
||||
**Parallelizable with:** Tasks 16, 17, 19, 20, 21, 22
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnWrapAlarmConsumer.cs` (~402–437 parse; ~50 `DefaultMaxAlarmsPerFetch`; ~323–330 snapshot rebuild; `ComputeTransitions` absence rule ~356)
|
||||
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessStaSession.cs` (~22 hard-coded 500 ms)
|
||||
- Modify: `docs/GatewayConfiguration.md`, `docs/DesignDecisions.md` (alarm sections)
|
||||
- Test: extend windev `MxAccess/` alarm-consumer tests — the truncation test is the important one
|
||||
|
||||
**Implement (three independent sub-changes):**
|
||||
1. **Parse cost:** in the per-alarm extraction, replace the ~14 `SelectSingleNode(child)` XPath calls with one pass over `alarmNode.ChildNodes` switching on `Name` (same fields, same defaults for absent children). Keep `XmlDocument` (an `XmlReader` rewrite is a bigger change than the win justifies once XPath is gone). Reuse the snapshot dictionary across polls (clear-and-refill → swap two dictionaries) only if trivially safe; otherwise skip — the XPath removal is the payload.
|
||||
2. **Truncation cliff (correctness fix):** when the fetch returns exactly `maxAlarmsPerFetch` records, treat the snapshot as **truncated**: log a warning (rate-limited, identifiers only) and suppress the absence-implies-Clear inference in `ComputeTransitions` for that poll (present alarms still update; nothing is cleared on the evidence of a capped fetch). Add the test: 1024-record fetch + a known alarm missing from it → no Clear transition emitted, warning logged.
|
||||
3. **Cadence + cap configurable:** plumb `MxGateway:Alarms:PollIntervalMilliseconds` (default 500, min 100) and `MaxAlarmsPerFetch` (default 1024) through the existing env-var pattern (as in Task 17). Gateway-side option + validator + launcher env, worker-side parse.
|
||||
|
||||
Commit: `fix(alarms): truncation-safe transitions; perf: single-pass alarm parse; configurable poll cadence`
|
||||
|
||||
---
|
||||
|
||||
### Task 24: Phase B verification on windev (psbridge)
|
||||
|
||||
**Classification:** high-risk (this is the gate for every Phase B commit)
|
||||
**Estimated implement time:** ~10 min wall
|
||||
**Parallelizable with:** none (after all Phase B tasks)
|
||||
|
||||
**Steps:**
|
||||
1. Invoke the `psbridge` skill and follow it (it covers exec/push/deploy against the Windows box).
|
||||
2. Push/pull the branch to windev (whatever the skill's established flow is — the repo has a remote the Windows box shares; `git pull` the branch there).
|
||||
3. On windev, run in order and capture output:
|
||||
```powershell
|
||||
dotnet build src/ZB.MOM.WW.MxGateway.slnx
|
||||
dotnet build src/ZB.MOM.WW.MxGateway.Worker/ZB.MOM.WW.MxGateway.Worker.csproj -p:Platform=x86
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Worker.Tests/ZB.MOM.WW.MxGateway.Worker.Tests.csproj -p:Platform=x86
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj
|
||||
```
|
||||
4. Any failure: fix on the Mac, commit, re-run the failed leg. Bisect by commit if the failure isn't obvious — Phase B commits are deliberately one-task-each.
|
||||
5. If psbridge is unreachable: STOP and report — Phase B remains "edited, unverified"; do not merge.
|
||||
|
||||
Live MXAccess smoke (`MXGATEWAY_RUN_LIVE_MXACCESS_TESTS=1`, `WorkerLiveMxAccessSmokeTests`) if provider state is available on windev; otherwise record why skipped, per `docs/GatewayTesting.md`.
|
||||
|
||||
---
|
||||
|
||||
### Task 25: Wrap-up — docs sweep, umbrella index, review deltas
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** none (last)
|
||||
|
||||
**Files:**
|
||||
- Verify each task's doc edits landed (`gateway.md`, `docs/Sessions.md`, `docs/GatewayConfiguration.md`, `docs/GatewayDashboardDesign.md`, `docs/DesignDecisions.md`, `docs/MxAccessWorkerInstanceDesign.md`, `docs/WorkerFrameProtocol.md`)
|
||||
- Modify: `../scadaproj/CLAUDE.md` — **only if** a fact the umbrella index records changed (new `MxGateway:Worker:EventQueueCapacity` / alarm options are config, not indexed facts; expected outcome: no umbrella change needed — verify, don't assume)
|
||||
- Check: no `.proto` diffs (`git diff main -- '*.proto'` must be empty)
|
||||
|
||||
Commit anything found: `docs: remediation plan doc sweep`
|
||||
|
||||
---
|
||||
|
||||
## Explicitly deferred (decided, not forgotten)
|
||||
|
||||
| Finding | Why deferred |
|
||||
|---|---|
|
||||
| Value-cache triple clone per `OnDataChange` | Removing the defensive copies needs a GWC-07-style aliasing audit across cache consumers; risk outweighs the win until profiled. |
|
||||
| net48 pipe-read cancellation | Benign in practice (worker exits after shutdown); a correct fix means restructuring stream teardown for a path that only fires at exit. |
|
||||
| Control-frame completion coupled to event batch drain | Documented, bounded (≤128 frames) behavior of the two-class writer design; revisit only if heartbeat latency shows up in metrics. |
|
||||
| Blazor pages' loopback SignalR hop | Works correctly; in-process `WatchSnapshotsAsync` consumption is a dashboard refactor with payoff only at viewer counts the product doesn't target. |
|
||||
| Event-path triple async-iterator flattening | LOW-rated; touches the most invariant-dense code in the gateway for two `MoveNextAsync` hops per event. Reconsider after Tasks 3/4 land and if profiling still shows it. |
|
||||
|
||||
## Execution notes for the orchestrator
|
||||
|
||||
- Branch: `git checkout -b perf/review-remediation` before Task 1.
|
||||
- Implementer subagents run on **Opus** per the user's instruction; reviewer chain per each task's Classification.
|
||||
- Parallel dispatch waves (no file overlap): **Wave 1:** 1, 3, 4, 5, 6, 8 · **Wave 2:** 2, 9, 10, 11, 12, 13, 14 · then 7 (after 2) · then 15 · **Wave 3 (Phase B):** 16, 18, 19, 20, 22, 23 · then 17, 21 (after 16) · then 24 · then 25. (Waves are a suggestion; the per-task `Parallelizable with` fields are the contract.)
|
||||
- Each implementer gets: its full task text, the ground rules block, and nothing else — the `Files:` block is the scope contract.
|
||||
@@ -0,0 +1,31 @@
|
||||
{
|
||||
"planPath": "docs/plans/2026-08-15-perf-review-remediation.md",
|
||||
"tasks": [
|
||||
{"id": 1, "subject": "Task 1: Named-pipe buffer sizes", "status": "pending"},
|
||||
{"id": 2, "subject": "Task 2: Metrics pull-gauge + Interlocked counters", "status": "pending"},
|
||||
{"id": 3, "subject": "Task 3: Distributor copy-on-write subscriber snapshot", "status": "pending"},
|
||||
{"id": 4, "subject": "Task 4: Dashboard event mirror viewer gating", "status": "pending"},
|
||||
{"id": 5, "subject": "Task 5: Snapshot pipeline idle gating + cached config", "status": "pending"},
|
||||
{"id": 6, "subject": "Task 6: Reply ownership transfer in MapCommandReply", "status": "pending"},
|
||||
{"id": 7, "subject": "Task 7: WorkerClient WaitAsync timeout + WorkerCancel", "status": "pending", "blockedBy": [2]},
|
||||
{"id": 8, "subject": "Task 8: Audit pipeline background writer + retention", "status": "pending"},
|
||||
{"id": 9, "subject": "Task 9: Parallel session teardown", "status": "pending"},
|
||||
{"id": 10, "subject": "Task 10: Dashboard live-data subscription cap", "status": "pending"},
|
||||
{"id": 11, "subject": "Task 11: Alarm monitor cached CurrentAlarms", "status": "pending"},
|
||||
{"id": 12, "subject": "Task 12: Logging middleware hoist + bearer redaction fix", "status": "pending"},
|
||||
{"id": 13, "subject": "Task 13: Auth-path span parsing + limiter keys", "status": "pending"},
|
||||
{"id": 14, "subject": "Task 14: Bulk constraint loops, caches, hygiene", "status": "pending"},
|
||||
{"id": 15, "subject": "Task 15: Phase A gate — full gateway suite", "status": "pending", "blockedBy": [1,2,3,4,5,6,7,8,9,10,11,12,13,14]},
|
||||
{"id": 16, "subject": "Task 16: Event drain wake signal", "status": "pending", "blockedBy": [15]},
|
||||
{"id": 17, "subject": "Task 17: Event queue capacity env plumbing", "status": "pending", "blockedBy": [16]},
|
||||
{"id": 18, "subject": "Task 18: STA message-driven completion waits", "status": "pending", "blockedBy": [15]},
|
||||
{"id": 19, "subject": "Task 19: Handle registry reverse index + O(1) removals", "status": "pending", "blockedBy": [15]},
|
||||
{"id": 20, "subject": "Task 20: Event conversion TryParseExact + compiled accessors", "status": "pending", "blockedBy": [15]},
|
||||
{"id": 21, "subject": "Task 21: Drain size memoized at enqueue", "status": "pending", "blockedBy": [16, 17]},
|
||||
{"id": 22, "subject": "Task 22: Worker frame writer/reader pooled buffers", "status": "pending", "blockedBy": [15]},
|
||||
{"id": 23, "subject": "Task 23: Alarm consumer parse + truncation + cadence", "status": "pending", "blockedBy": [15]},
|
||||
{"id": 24, "subject": "Task 24: Phase B verification on windev (psbridge)", "status": "pending", "blockedBy": [16,17,18,19,20,21,22,23]},
|
||||
{"id": 25, "subject": "Task 25: Wrap-up docs sweep + umbrella check", "status": "pending", "blockedBy": [24]}
|
||||
],
|
||||
"lastUpdated": "2026-08-15T00:00:00Z"
|
||||
}
|
||||
Reference in New Issue
Block a user