From 3619152b9f0664d45bf565c98c5affe38ad9d6c6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:23:36 -0400 Subject: [PATCH] chore(GWC-31): relabel perf-pass GWC-3x comments to PERF-nn; tracker housekeeping Sweep code/doc comments citing unregistered GWC-30/GWC-31 (2026-08-15 perf pass) to PERF-02/PERF-07/PERF-22, keyed to task numbers in docs/plans/2026-08-15-perf-review-remediation.md; add a PERF-ID register table to that plan. Genuine cycle-2 GWC-30 citations (gateway-side frame- reader 4-byte prefix fix in Server/Workers/WorkerFrameReader.cs and its test) are left untouched. Add a line to REVIEW-PROCESS.md reserving the GWC/WRK/IPC/SEC/CLI/TST prefixes for the archreview domain registers. Tracker housekeeping per archreview/2026-08-19/00-overall.md: - GWC-16 -> Won't fix (rationale: PERF(followup) comment documents the double session lookup as measured-negligible, deliberately not collapsed) - IPC-08 -> Done (closed by 7c1ea12/a1a38b5; residual tracked as IPC-34) - IPC-31 -> Done (superseded by GWC-28's write-time sequence stamping) - TST-06, TST-17 rows refreshed per cycle-3 60-testing-docs-gaps.md - WRK-31 (and defensively WRK-29/30) registered as reserved/do-not-reuse interim IDs burned by e913dab, in both the cycle-2 tracker and the cycle-2 worker remediation doc Flip Batch C to Done in archreview/2026-08-19/remediation/00-tracking.md. Worker-side files (Worker/Ipc/WorkerFrameReader.cs, WorkerFrameWriter.cs) are comment-only edits, unverified here (net48 x86, windev-only build). --- REVIEW-PROCESS.md | 6 ++++++ .../2026-07-12/remediation/00-tracking.md | 5 ++++- archreview/2026-07-12/remediation/20-worker.md | 1 + .../2026-07-12/remediation/30-contracts-ipc.md | 2 ++ .../2026-08-19/remediation/00-tracking.md | 2 +- archreview/remediation/00-tracking.md | 8 ++++---- archreview/remediation/10-gateway-core.md | 2 ++ archreview/remediation/30-contracts-ipc.md | 2 ++ archreview/remediation/60-testing-docs-gaps.md | 4 ++++ docs/Metrics.md | 4 ++-- docs/plans/2026-08-15-deferred-remediation.md | 2 +- .../2026-08-15-perf-review-remediation.md | 18 ++++++++++++++++++ .../Metrics/GatewayMetrics.cs | 10 +++++----- .../Workers/WorkerClient.cs | 8 ++++---- .../Gateway/Workers/WorkerClientTests.cs | 4 ++-- .../Metrics/GatewayMetricsTests.cs | 4 ++-- .../Ipc/WorkerFrameReader.cs | 2 +- .../Ipc/WorkerFrameWriter.cs | 8 ++++---- 18 files changed, 65 insertions(+), 27 deletions(-) diff --git a/REVIEW-PROCESS.md b/REVIEW-PROCESS.md index f53825d..11540d4 100644 --- a/REVIEW-PROCESS.md +++ b/REVIEW-PROCESS.md @@ -79,6 +79,12 @@ Add one entry per finding to the `## Findings` section of the module's - **Finding ID** — `-NNN`, numbered sequentially within the module and never reused (e.g. `Worker-001`). IDs are permanent even after resolution. + The `GWC`/`WRK`/`IPC`/`SEC`/`CLI`/`TST` prefixes are reserved for the + `archreview/` domain registers (a separate, whole-repo review series with its + own tracking documents) — never assign one of these prefixes to a + `code-reviews/` finding, and any other work stream (perf passes, ad hoc + remediation plans, etc.) that wants a stable per-item ID must mint its own + distinct prefix rather than borrowing one of these six. - **Severity:** - **Critical** — data loss, security breach, crash/deadlock, or outage. - **High** — incorrect behaviour with significant impact; no safe workaround. diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index 169b4aa..c4c6d57 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -79,6 +79,9 @@ Full design + implementation for each row lives in the linked domain doc under i | WRK-26 | Low | P1 | S | WRK-23 (soft); discharges IPC-29 | Done | Write-priority and overflow doc drift from the WRK-07 change | | WRK-27 | Low | — | S | — | Done | Alarm poll bypasses the watchdog's in-flight suppression (15 s vs 75 s) | | WRK-28 | Low | — | S | WRK-21 (same batch) | Done | 10,000 drain cap is a duplicated magic constant | +| WRK-29 | — | — | — | — | Reserved (do not reuse) | Interim ID space possibly burned alongside WRK-31 by `e913dab`'s ad hoc numbering; not confirmed in the repo or its history, held back defensively | +| WRK-30 | — | — | — | — | Reserved (do not reuse) | Interim ID space possibly burned alongside WRK-31 by `e913dab`'s ad hoc numbering; not confirmed in the repo or its history, held back defensively | +| WRK-31 | — | — | — | — | Reserved (do not reuse) | Interim ID burned by `e913dab` (session-owned transport disposal), cited in code at `Ipc/WorkerPipeSession.cs:45,177` and `Ipc/WorkerPipeSessionTests.cs` outside the register; not a finding — registered per cycle-3 `20-worker.md` (~line 9) so it is never reassigned. Cycle-3 new findings start at WRK-32. | ### Contracts & IPC — [30-contracts-ipc.md](30-contracts-ipc.md) @@ -92,7 +95,7 @@ Full design + implementation for each row lives in the linked domain doc under i | IPC-28 | Low | — | S | — | Done | docs/Grpc.md missing CommandTooLarge → ResourceExhausted mapping | | IPC-29 | Low | — | S | WRK-26 (discharged by) | Done (discharged by WRK-26) | WorkerFrameProtocol.md missing write-scheduling/sequencing section | | IPC-30 | Low | P0 | M | WRK-21 (same batch) | Done | Oversized event frame: keep session-fatal, make the death structured | -| IPC-31 | Info | — | — | — | N/A | Gateway creation-time sequence stamping accepted; diagnostic-only, decision recorded | +| IPC-31 | Info | — | — | — | Done (superseded by GWC-28) | Gateway creation-time sequence stamping — resolved when GWC-28 moved gateway stamping to write time; the two sides now implement the same semantics | | IPC-32 | Info | — | S | IPC-25 (folded in) | Done | check-codegen banner relabel 1/4…4/4 | ### Security & dashboard — [40-security-dashboard.md](40-security-dashboard.md) diff --git a/archreview/2026-07-12/remediation/20-worker.md b/archreview/2026-07-12/remediation/20-worker.md index 93ab57f..c00e482 100644 --- a/archreview/2026-07-12/remediation/20-worker.md +++ b/archreview/2026-07-12/remediation/20-worker.md @@ -24,6 +24,7 @@ members, no positional records). The worker builds and tests only on the Windows | WRK-26 | Low | P1 | S | WRK-23 (soft — sequence prose); discharges IPC-29 | Done | Write-priority and overflow doc drift from the WRK-07 change | | WRK-27 | Low | — | S | — | Done | Alarm poll bypasses the watchdog's in-flight suppression (15 s vs 75 s) | | WRK-28 | Low | — | S | WRK-21 (land in the same commit cluster) | Done | 10,000 drain cap is a duplicated magic constant with a comment-only sync contract | +| WRK-31 | — | — | — | — | Reserved (do not reuse) | Not a finding. `e913dab` (session-owned transport disposal) coined "WRK-31" ad hoc in code comments (`Ipc/WorkerPipeSession.cs:45,177`) and in `Ipc/WorkerPipeSessionTests.cs`, before the register assigned it. WRK-29/WRK-30 appear nowhere in the repo or its history but may have been burned by the same interim numbering, so they are held back defensively too — see the tracker's WRK-29/30/31 rows. Registered here per cycle-3 `archreview/2026-08-19/20-worker.md` (~line 9, "Finding-ID note"); that review's new findings start at **WRK-32** to guarantee no collision. | --- diff --git a/archreview/2026-07-12/remediation/30-contracts-ipc.md b/archreview/2026-07-12/remediation/30-contracts-ipc.md index cb97d99..5c4216e 100644 --- a/archreview/2026-07-12/remediation/30-contracts-ipc.md +++ b/archreview/2026-07-12/remediation/30-contracts-ipc.md @@ -231,6 +231,8 @@ The published-side collection (`CollectPublishedSymbols`, `:61-79`) grows matchi **Disposition: N/A — no action.** `gateway.md:328-330` already declares sequence a per-sender diagnostic aid, never validated; both validators ignore it; the divergence is harmless and now explicitly documented by the IPC-29 doc section (which states the two sides' stamping points). If sequence ever becomes load-bearing, stamp in the gateway write loop (`WorkerClient.cs:390-397`) — recorded here so the next editor finds the decision. +**Resolution: Done — superseded by GWC-28 (2026-08-20, per cycle-3 verification).** GWC-28 moved the gateway-side stamp from creation to the point of writing: `WriteLoopAsync` now stamps `Sequence` immediately before `_writer.WriteAsync` (`Server/Workers/WorkerClient.cs:465-471`), and `CreateEnvelope` leaves it unset (`:1229-1236`); `gateway.md:425-431` was rewritten to "both sides stamp at the point of writing", closing the divergence this finding recorded. Regression test `WorkerClientTests.ConcurrentInvokesEmitStrictlyIncreasingSequencesOnTheWire` covers it. Per `archreview/2026-08-19/30-contracts-ipc.md` (IPC-31 row), tracker flipped from `N/A` to `Done (superseded by GWC-28)`. See cycle-3 **IPC-39** for the one interaction the change created with the IPC-03 pre-check. + --- ## IPC-32 — `check-codegen.ps1` check labels are miscounted `Info` · `—` (rides IPC-25) diff --git a/archreview/2026-08-19/remediation/00-tracking.md b/archreview/2026-08-19/remediation/00-tracking.md index 1257225..d360af0 100644 --- a/archreview/2026-08-19/remediation/00-tracking.md +++ b/archreview/2026-08-19/remediation/00-tracking.md @@ -15,7 +15,7 @@ This plan is executed as **seven parallel batches**, each by one subagent workin |---|---|---|---|---|---| | A | `fix/c3-client-versions` | CLI-46 | sonnet | Mechanical five-client bump + README note + one script guard; recipe is fully specified | Not started | | B | `fix/c3-glauth-doc` | TST-31 | sonnet | Reconcile a table against one TOML source of truth, re-derive one default, change an assert to a log | Not started | -| C | `fix/c3-tracker-hygiene` | GWC-31 + housekeeping (close GWC-16/IPC-08/IPC-31, refresh TST-06/TST-17, register WRK-31) | sonnet | Cross-file comment/ID sweep needs care about what each ID *means*, but no logic changes | Not started | +| C | `fix/c3-tracker-hygiene` | GWC-31 + housekeeping (close GWC-16/IPC-08/IPC-31, refresh TST-06/TST-17, register WRK-31) | sonnet | Cross-file comment/ID sweep needs care about what each ID *means*, but no logic changes | Done | | D | `fix/c3-dashboard-acl` | SEC-40, SEC-41, GWC-38 | opus | Authorization semantics, fail-closed decisions, claims re-resolution; each needs a regression test | Not started | | E | `fix/c3-ipc-boundaries` | IPC-33, IPC-39, IPC-34, IPC-35 | opus | Changes the WorkerClient command lifecycle, the options validator, and the worker drain budget; race-sensitive | Not started | | F | `fix/c3-worker-residuals` | WRK-32, WRK-33 (+WRK-34 opportunistic) | sonnet | Small, precisely located worker edits + doc prose; cannot be built on macOS so the agent's job is a careful, minimal diff | Not started | diff --git a/archreview/remediation/00-tracking.md b/archreview/remediation/00-tracking.md index 258549d..d49dfdb 100644 --- a/archreview/remediation/00-tracking.md +++ b/archreview/remediation/00-tracking.md @@ -74,7 +74,7 @@ Full design + implementation for each row lives in the linked domain doc under i | GWC-13 | Low | — | M | — | Not started | Worker-ready wait is a 25 ms poll loop | | GWC-14 | Low | P2 | M | — | Done | Replay ring is a `LinkedList` with a node alloc per event | | GWC-15 | Low | P2 | S | — | Done | Per-event gauge reads `ChannelReader.Count` every event | -| GWC-16 | Low | — | S | — | Not started | `Invoke` resolves the session twice per command | +| GWC-16 | Low | — | S | — | Won't fix | `Invoke` resolves the session twice per command — measured negligible, deliberately not collapsed (see domain doc) | | GWC-17 | Low | — | M | — | Not started | Sessions layer throws `Grpc.Core.RpcException` (layering leak) | | GWC-18 | Low | — | S | — | Not started | `GatewaySession` implements `DisposeAsync` without `IAsyncDisposable` | | GWC-19 | Low | — | S | — | Not started | Dead `GatewaySession.KillWorker(string)` + stale doc | @@ -119,7 +119,7 @@ Full design + implementation for each row lives in the linked domain doc under i | IPC-05 | Medium | P2 | S | — | Done | Redundant deep copies on command and event hot paths | | IPC-06 | Medium | P2 | S | — | Done | `gateway.md` Worker Envelope sketch no longer matches the contract | | IPC-07 | Medium | P2 | S | — | Done | `docs/Grpc.md` says six RPCs; there are seven | -| IPC-08 | Medium | — | M | — | Not started | `WorkerCancel` defined and handled but never sent | +| IPC-08 | Medium | — | M | — | Done | `WorkerCancel` defined and handled but never sent — resolved by `7c1ea12`/`a1a38b5`; residual (caller-cancellation leg) tracked as cycle-3 IPC-34 | | IPC-09 | Medium | P1 | M | IPC-01 | Done | Known codegen fragilities have no in-repo guards | | IPC-10 | Low | — | S | — | Not started | Envelope `sequence` is write-only; monotonicity never validated | | IPC-11 | Low | — | S | — | Not started | No protocol version negotiation despite `supported_protocol_version` name | @@ -218,7 +218,7 @@ Full design + implementation for each row lives in the linked domain doc under i | TST-03 | High | P1 | M | — | Done | No CI exists | | TST-04 | High | P2 | L | — | Done | Session-resilience epic 16/28 tasks unfinished | | TST-05 | Medium | P1 | S | TST-03 | Partially done | Real-worker control/COM paths verified opt-in only. **Scheduling half closed 2026-08-10** by the TST-25 `nightly-windev` job (`.gitea/workflows/ci.yml`, cron `0 6 * * *` → `scripts/ci/run-windev-ci.sh live`, which sets `MXGATEWAY_RUN_LIVE_MXACCESS_TESTS=1`, runs `WorkerLiveMxAccessSmokeTests`, and opens a Gitea issue on failure) — "opt-in, run by memory" is now "runs nightly, reports failures". **Residual: the coverage-audit half.** The live suite's 8 facts cover all six late-added COM commands but none of the five control commands (`Ping`, `GetSessionState`, `GetWorkerInfo`, `DrainEvents`, `ShutdownWorker`), which real workers answer in `Worker/Ipc/WorkerPipeSession.cs` yet are still only exercised through `FakeWorkerHarness` canned replies — precisely the masking the finding named | -| TST-06 | Medium | — | M | — | Not started | Dashboard live-data path untested | +| TST-06 | Medium | — | M | — | Materially narrowed | Dashboard live-data path untested — `DashboardLiveDataServiceTests`, `EventsHubTests`, `EventsHubViewerRegistryTests`, `AlarmsHubPublisherTests`, `SessionDetailsPageEventAclTests` now exist (landed with SEC-25/TST-16); needs a re-audit-and-close pass, not re-implementation | | TST-07 | Medium | — | S | — | Not started | Real-clock sleeps with negative assertions are latent flakes | | TST-08 | Medium | P1 | M | — | Done | Full-suite orphaned testhost processes (does not reproduce; doc de-stale) | | TST-09 | Medium | — | M | — | Not started | Health checks cover only the auth store | @@ -229,7 +229,7 @@ Full design + implementation for each row lives in the linked domain doc under i | TST-14 | Medium | P2 | S | — | Not started | Repo-root working artifacts need triage | | TST-15 | Medium | P2 | M | TST-04 | Done | Dashboard EventsHub has no per-session ACL | | TST-16 | Medium | P1 | S | — | Done | `Dashboard:ShowTagValues` is a dead flag | -| TST-17 | Medium | — | S | — | Not started | Vendor-gated alarm parity residuals silently lossy | +| TST-17 | Medium | — | S | — | Materially changed | Vendor-gated alarm parity residuals silently lossy — `WnWrapAlarmConsumer.cs:309-327` now passes `szDomainName`/`szOprFullName`; the vendor 8-arg leg is recorded "unavailable by configuration" with evidence (`docs/AlarmProbeFindings.md`, `d1ae43d`, `540a118`); "silent drop, unchanged" is no longer accurate — needs a worker-domain refresh pass | | TST-18 | Low | — | S | — | Not started | Hosted-service wrappers untested | | TST-19 | Low | — | S | — | Not started | Keep FakeWorkerHarness canned replies in lockstep | | TST-20 | Low | — | S | — | Not started | E2E script papers over a real advise-without-consumer sharp edge | diff --git a/archreview/remediation/10-gateway-core.md b/archreview/remediation/10-gateway-core.md index 8121ba3..042fb0a 100644 --- a/archreview/remediation/10-gateway-core.md +++ b/archreview/remediation/10-gateway-core.md @@ -330,6 +330,8 @@ Open question the review cannot settle: whether a future probe (COM-init readine **Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~SessionManager"`. +**Resolution: Won't fix (2026-08-20, closed by cycle-3 verification).** `Grpc/MxAccessGatewayService.cs:111-115` carries a `PERF(followup)` comment documenting the double session lookup as measured-negligible and deliberately not collapsed — the fix would duplicate `InvokeAsync`'s fault mapping at a second entry point, trading a negligible per-command cost for a maintenance liability. Per the cycle-3 gateway-core review (`archreview/2026-08-19/10-gateway-core.md`, GWC-16 row), the rationale is sound; tracker flipped from `Not started` to `Won't fix`. + --- ## GWC-17 — Sessions layer throws `Grpc.Core.RpcException` `Low` · `—` diff --git a/archreview/remediation/30-contracts-ipc.md b/archreview/remediation/30-contracts-ipc.md index f8115f7..1c5f224 100644 --- a/archreview/remediation/30-contracts-ipc.md +++ b/archreview/remediation/30-contracts-ipc.md @@ -175,6 +175,8 @@ The frame protocol and proto evolution hygiene are sound; the remediable risk cl **Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` + `dotnet test ...Tests --filter FullyQualifiedName~WorkerClient`; worker side `-p:Platform=x86`. +**Resolution: Done (2026-08-20, closed by cycle-3 verification, residual filed).** Wired up substantially as designed: `InvokeAsync` forwards a `WorkerCancel` when the gateway's own command timeout fires and it owned the removal (`Server/Workers/WorkerClient.cs:269-291`), `TrySendCancelForTimedOutCommand` is structurally no-throw (`:835-870`), the worker routes it to `CancelCommand` (`Worker/Ipc/WorkerPipeSession.cs:755-756` → `MxAccess/MxAccessStaSession.cs:529-532` → `Sta/StaCommandDispatcher.CancelQueuedCommand:151`), regression test `WorkerClientTests.InvokeAsync_WhenCommandTimesOut_SendsWorkerCancelForThatCorrelation` exists, and `docs/GatewayProcessDesign.md:596-602` documents it. Landed in `7c1ea12`/`a1a38b5`. **Residual, not covered by this closure:** the caller-cancellation leg (context deadline / client disconnect, as opposed to the gateway's own `CommandTimeout`) sends nothing — filed as cycle-3 **IPC-34**. Per `archreview/2026-08-19/30-contracts-ipc.md` (IPC-08 row), tracker flipped from `Not started` to `Done` with the residual cross-referenced. + --- ## IPC-09 — Known codegen fragilities have no in-repo guards `Medium` · `P1` diff --git a/archreview/remediation/60-testing-docs-gaps.md b/archreview/remediation/60-testing-docs-gaps.md index 6ebf9cd..e585765 100644 --- a/archreview/remediation/60-testing-docs-gaps.md +++ b/archreview/remediation/60-testing-docs-gaps.md @@ -191,6 +191,8 @@ This finding is the umbrella; TST-01/02/15 are its actionable slices. The remedi **Verification.** `dotnet test ... --filter FullyQualifiedName~DashboardLiveDataServiceTests`. +**Status refresh (2026-08-20, per cycle-3 verification).** Materially narrowed — the tracker row was stale. `Tests/Gateway/Dashboard/DashboardLiveDataServiceTests.cs`, `EventsHubTests.cs`, `EventsHubViewerRegistryTests.cs`, `AlarmsHubPublisherTests.cs`, and `SessionDetailsPageEventAclTests.cs` all exist now (mostly landed with the SEC-25/TST-16 closeouts and the in-process dashboard-feed work). Per `archreview/2026-08-19/60-testing-docs-gaps.md` (TST-06 row), this needs a re-audit-and-close pass against the current coverage rather than fresh implementation — left open pending that audit, tracker status changed from `Not started` to `Materially narrowed` to reflect the actual state. + --- ## TST-07 — Real-clock sleeps with negative assertions are latent flakes `Medium` · `—` @@ -371,6 +373,8 @@ If TST-02's interim mitigation (flip retention off) is chosen instead of impleme **Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Worker -p:Platform=x86`; `dotnet test src/ZB.MOM.WW.MxGateway.Worker.Tests/... -p:Platform=x86` (Windows). +**Status refresh (2026-08-20, per cycle-3 verification).** Materially changed — the tracker row needs a refresh from the worker domain. `WnWrapAlarmConsumer.cs:309-327` now passes `szDomainName`/`szOprFullName` into the COM call (the `_ = ackOperatorDomain` discard is gone), and the 2026-08 probe cycle recorded the vendor 8-arg leg as "unavailable by configuration" with evidence (`docs/AlarmProbeFindings.md`, commits `d1ae43d`, `540a118`). Per `archreview/2026-08-19/60-testing-docs-gaps.md` (TST-17 row), "silent drop, unchanged" is no longer an accurate description of the row; whether a caller-visible degrade signal now exists is the worker reviewer's call to close out. Tracker status changed from `Not started` to `Materially changed`. + --- ## TST-18 — Hosted-service wrappers untested `Low` · `—` diff --git a/docs/Metrics.md b/docs/Metrics.md index c13ea91..e3b9bf6 100644 --- a/docs/Metrics.md +++ b/docs/Metrics.md @@ -72,7 +72,7 @@ Observable gauges are pull-based; the `Meter` invokes the supplied callback when |------------|--------------|-------------| | `mxgateway.sessions.open` | `_openSessions` | Currently open sessions tracked by `SessionManager`. | | `mxgateway.workers.running` | `_workersRunning` | Worker clients in a running state. | -| `mxgateway.events.worker_queue.depth` | `_workerEventQueueDepthSources` (summed on demand) | Undelivered worker events held by `WorkerClient` — staged *and* queued (GWC-24) — summed across every live client at collection time (GWC-30). Each client owns an interlocked counter incremented when the read loop stages an event and decremented when the consumer reads it, and registers it as a gauge source for its lifetime, so a backlog stuck in a staging channel is visible and concurrent sessions add up instead of overwriting one another. | +| `mxgateway.events.worker_queue.depth` | `_workerEventQueueDepthSources` (summed on demand) | Undelivered worker events held by `WorkerClient` — staged *and* queued (GWC-24) — summed across every live client at collection time (PERF-02). Each client owns an interlocked counter incremented when the read loop stages an event and decremented when the consumer reads it, and registers it as a gauge source for its lifetime, so a backlog stuck in a staging channel is visible and concurrent sessions add up instead of overwriting one another. | | `mxgateway.events.grpc_stream_queue.depth` | `_eventStreamBacklogSources` (summed on demand) | Live backlog buffered across every active `EventStreamService` subscriber, summed from the subscribers' channel `Count` at collection time. | ## Snapshot Shape @@ -111,7 +111,7 @@ The scalar fields mirror the counters and gauges. The four dictionaries provide - `EventsBySession` keys by `sessionId`; entries are removed via `RemoveSessionEvents` when a session closes so the map does not grow without bound. - `RetryAttemptsByArea` keys by the resilience `area` tag, e.g. `worker_startup`. -`EventsReceived` is read with `Interlocked.Read(ref _eventsReceived)` because `EventReceived` increments it via `Interlocked.Increment` outside the lock to keep the event-ingestion path non-blocking. `CommandsStarted`, `CommandsSucceeded`, `CommandsFailed`, and `CommandFailuresByMethod` are read the same way: the command counters run two-to-three times per gRPC call, so they are recorded with `Interlocked` and a `ConcurrentDictionary` rather than under `_syncRoot` (GWC-30). The two queue depths are pulled from their registered sources before the lock is taken, since those delegates reach into subscriber channels and worker clients. +`EventsReceived` is read with `Interlocked.Read(ref _eventsReceived)` because `EventReceived` increments it via `Interlocked.Increment` outside the lock to keep the event-ingestion path non-blocking. `CommandsStarted`, `CommandsSucceeded`, `CommandsFailed`, and `CommandFailuresByMethod` are read the same way: the command counters run two-to-three times per gRPC call, so they are recorded with `Interlocked` and a `ConcurrentDictionary` rather than under `_syncRoot` (PERF-02). The two queue depths are pulled from their registered sources before the lock is taken, since those delegates reach into subscriber channels and worker clients. ## Recording Sites diff --git a/docs/plans/2026-08-15-deferred-remediation.md b/docs/plans/2026-08-15-deferred-remediation.md index a01c5a2..4058674 100644 --- a/docs/plans/2026-08-15-deferred-remediation.md +++ b/docs/plans/2026-08-15-deferred-remediation.md @@ -241,7 +241,7 @@ Run `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx` (expect 0 warnings) a - Modify: `src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs` - Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs` (comment only) -**Why:** On net48, `NamedPipeClientStream.ReadAsync` ignores its `CancellationToken` (`WorkerFrameReader.cs:109-111`). Fault-path exits (event-drain fault, oversized event, watchdog, heartbeat write failure) leave `readTask` pending; it is unblocked only when `WorkerPipeClient`'s `using` disposes the pipe, at which point it faults with `ObjectDisposedException`/`IOException` on a Task nobody observes (the finally at `:303-309` awaits only heartbeat and drain). The frame-pooling change (GWC-30) makes this sharper: the abandoned read owns the per-instance `_lengthPrefix` buffer and possibly a rented ArrayPool payload — the reader's single-consumer invariant holds today only because nothing ever reads again after abandonment. +**Why:** On net48, `NamedPipeClientStream.ReadAsync` ignores its `CancellationToken` (`WorkerFrameReader.cs:109-111`). Fault-path exits (event-drain fault, oversized event, watchdog, heartbeat write failure) leave `readTask` pending; it is unblocked only when `WorkerPipeClient`'s `using` disposes the pipe, at which point it faults with `ObjectDisposedException`/`IOException` on a Task nobody observes (the finally at `:303-309` awaits only heartbeat and drain). The frame-pooling change (PERF-22) makes this sharper: the abandoned read owns the per-instance `_lengthPrefix` buffer and possibly a rented ArrayPool payload — the reader's single-consumer invariant holds today only because nothing ever reads again after abandonment. **Spec — constraints, implementer designs within them:** 1. **No unobserved faulted Task.** After the stream is disposed, `readTask`'s fault must be awaited/observed (reuse `ObserveBackgroundTaskStopAsync`'s timeout-and-log shape, `:312-348`) before `WorkerPipeClient.RunAsync` returns. diff --git a/docs/plans/2026-08-15-perf-review-remediation.md b/docs/plans/2026-08-15-perf-review-remediation.md index 83a975e..21ca92a 100644 --- a/docs/plans/2026-08-15-perf-review-remediation.md +++ b/docs/plans/2026-08-15-perf-review-remediation.md @@ -608,6 +608,24 @@ Commit anything found: `docs: remediation plan doc sweep` --- +## PERF-ID register + +This plan's tasks were cited in code comments and test names as `GWC-30`/`GWC-31` before +those labels were recognized as colliding with the `archreview` finding register — see +cycle-3 finding `GWC-31` (`archreview/2026-08-19/10-gateway-core.md`). The comments were +swept to the `PERF-nn` prefix below (`nn` = this doc's task number); `GWC-30`/`GWC-31` no +longer appear in perf-pass comments. Citations of the *genuine* archreview `GWC-30` +("Frame reader still allocates a fresh 4-byte prefix array per frame", +`archreview/2026-07-12/10-gateway-core.md`) — the gateway-side +`Server/Workers/WorkerFrameReader.cs` fix and its test — were left untouched; they refer +to a real register entry, not this plan. + +| PERF-ID | Task | Files | +|---|---|---| +| PERF-02 | Task 2 — Metrics: pull-gauge for worker queue depth, lock-free command counters | `Server/Workers/WorkerClient.cs` (`:44`, `:355`), `Server/Metrics/GatewayMetrics.cs` (`:46`, `:213`, `:284`, `:463`, `:524`), `Tests/Metrics/GatewayMetricsTests.cs` (`:22`, `:61`), `docs/Metrics.md` | +| PERF-07 | Task 7 — WorkerClient: pooled-timer timeout, single sizing pass, `WorkerCancel` on timeout | `Server/Workers/WorkerClient.cs` (`:236`, `:253`), `Tests/Gateway/Workers/WorkerClientTests.cs` (`:163`, `:179`) | +| PERF-22 | Task 22 — Worker frame writer/reader: pooled buffers | `Worker/Ipc/WorkerFrameReader.cs` (`:23`), `Worker/Ipc/WorkerFrameWriter.cs` (`:671`), `docs/plans/2026-08-15-deferred-remediation.md` (Task cross-reference) | + ## Explicitly deferred (decided, not forgotten) | Finding | Why deferred | diff --git a/src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs b/src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs index 36e8938..3f1a7dc 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs @@ -43,7 +43,7 @@ public sealed class GatewayMetrics : IDisposable private readonly ConcurrentDictionary> _eventStreamBacklogSources = new(); private long _nextEventStreamBacklogSourceId; - // GWC-30: the same pull model for the worker event queue depth. It replaces a pushed scalar that + // PERF-02: the same pull model for the worker event queue depth. It replaces a pushed scalar that // every WorkerClient wrote twice per event (staged, consumed) under _syncRoot — a process-wide // lock on the hottest path, and last-writer-wins across sessions, so the gauge reported one // arbitrary session's backlog instead of the gateway's. Each client registers a source returning @@ -210,7 +210,7 @@ public sealed class GatewayMetrics : IDisposable /// Name of the command method. public void CommandStarted(string method) { - // GWC-30: the three command counters run two-to-three times per gRPC call, so they use + // PERF-02: the three command counters run two-to-three times per gRPC call, so they use // Interlocked rather than _syncRoot — the same idiom as EventReceived. Nothing here needs a // consistent multi-field view; GetSnapshot reads each with Interlocked.Read. Interlocked.Increment(ref _commandsStarted); @@ -281,7 +281,7 @@ public sealed class GatewayMetrics : IDisposable /// Registers a live depth source for the worker event queue-depth gauge and returns a handle /// that removes it when disposed. Each WorkerClient registers once and reports its own /// undelivered (staged + queued) event count, so the gauge is the gateway-wide sum instead of - /// the last value any one session happened to push (GWC-30). + /// the last value any one session happened to push (PERF-02). /// /// /// Returns this worker client's current undelivered event count. Invoked only at collection @@ -460,7 +460,7 @@ public sealed class GatewayMetrics : IDisposable { // Compute the live queue depths outside _syncRoot: the sources are the subscriber channels' // Count (their own locks) and the worker clients' interlocked counters, neither of which may - // run under this lock. GWC-15, GWC-30. + // run under this lock. GWC-15, PERF-02. int workerEventQueueDepth = GetWorkerEventQueueDepth(); int grpcEventStreamQueueDepth = GetGrpcEventStreamQueueDepth(); lock (_syncRoot) @@ -521,7 +521,7 @@ public sealed class GatewayMetrics : IDisposable } } - // Sums the undelivered event backlog across every live worker client (GWC-30). + // Sums the undelivered event backlog across every live worker client (PERF-02). private int GetWorkerEventQueueDepth() => SumSources(_workerEventQueueDepthSources); // Sums the live backlog across every registered event-stream subscriber. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs b/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs index bff47b1..1e9276f 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs @@ -41,7 +41,7 @@ public sealed class WorkerClient : IWorkerClient private readonly SemaphoreSlim _pendingCommandSlots; private readonly CancellationTokenSource _stopCts = new(); - // GWC-30: this client's contribution to the gateway-wide worker queue-depth gauge. Registered + // PERF-02: this client's contribution to the gateway-wide worker queue-depth gauge. Registered // once here and read only when the gauge is scraped, so staging and consuming an event cost an // Interlocked on _eventQueueDepth and nothing else — previously each of those two hot-path steps // called into GatewayMetrics and took its process-wide lock. Null when metrics are disabled. @@ -233,7 +233,7 @@ public sealed class WorkerClient : IWorkerClient // size the caller controls; checking here keeps a MessageTooLarge in the write loop a // genuine desync signal. // - // PERF(GWC-31): this size cannot be handed to WorkerFrameWriter to spare its own + // PERF-07: this size cannot be handed to WorkerFrameWriter to spare its own // CalculateSize. WriteLoopAsync stamps envelope.Sequence immediately before the write // (GWC-28), and a non-zero varint field grows the encoding — so the number computed here // is a lower bound on the frame the writer actually emits, never the frame length. Passing @@ -250,7 +250,7 @@ public sealed class WorkerClient : IWorkerClient await EnqueueAsync(commandEnvelope, cancellationToken).ConfigureAwait(false); - // GWC-31: one pooled timer instead of a linked CTS + Task.Delay + WhenAny per command. + // PERF-07: one pooled timer instead of a linked CTS + Task.Delay + WhenAny per command. // Task.WaitAsync arms a TimerQueueTimer on the shared timer queue and cancels it when the // reply lands, so the steady-state cost of a command that replies in time is a single // continuation — the old shape allocated a linked CancellationTokenSource, its @@ -352,7 +352,7 @@ public sealed class WorkerClient : IWorkerClient { await foreach (WorkerEvent workerEvent in _events.Reader.ReadAllAsync(cancellationToken).ConfigureAwait(false)) { - // No metrics call on the hot path: the gauge pulls _eventQueueDepth when scraped (GWC-30). + // No metrics call on the hot path: the gauge pulls _eventQueueDepth when scraped (PERF-02). Interlocked.Decrement(ref _eventQueueDepth); yield return workerEvent; } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs index 807249d..38e38d6 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs @@ -160,7 +160,7 @@ public sealed class WorkerClientTests TestTimeout, CancellationToken.None); - // The timeout also emits a WorkerCancel for the abandoned correlation (GWC-31), which sits + // The timeout also emits a WorkerCancel for the abandoned correlation (PERF-07), which sits // ahead of the second command on the FIFO pipe; skip it rather than mistaking it for the // command this assertion is about. WorkerEnvelope secondCommand = await ReadNextCommandAsync(pipePair, timedOutCommand.CorrelationId); @@ -176,7 +176,7 @@ public sealed class WorkerClientTests /// /// A command timeout abandons the gateway-side wait, but the worker keeps the correlation on /// its single STA queue and would still run it — so the gateway forwards a WorkerCancel - /// for the abandoned correlation id (GWC-31). Without it, a client that retries after a + /// for the abandoned correlation id (PERF-07). Without it, a client that retries after a /// timeout stacks work the worker still intends to execute. Asserted on the wire because the /// cancel is protocol behavior the worker depends on, not an internal detail. /// diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Metrics/GatewayMetricsTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Metrics/GatewayMetricsTests.cs index 5d67920..4101600 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Metrics/GatewayMetricsTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Metrics/GatewayMetricsTests.cs @@ -19,7 +19,7 @@ public sealed class GatewayMetricsTests metrics.CommandFailed("WriteSecured", "AuthorizationFailed", TimeSpan.FromMilliseconds(12)); metrics.EventReceived("session-1", "OnDataChange"); metrics.EventReceived("session-1", "OnDataChange"); - // GWC-30: the worker queue-depth gauge sums one live source per worker client, so the two + // PERF-02: the worker queue-depth gauge sums one live source per worker client, so the two // registrations below stand in for two concurrent sessions holding 3 and 4 events. using IDisposable workerDepthSourceA = metrics.RegisterWorkerEventQueueDepthSource(static () => 3); using IDisposable workerDepthSourceB = metrics.RegisterWorkerEventQueueDepthSource(static () => 4); @@ -58,7 +58,7 @@ public sealed class GatewayMetricsTests } /// - /// GWC-30: the worker queue-depth gauge sums every registered source rather than holding a + /// PERF-02: the worker queue-depth gauge sums every registered source rather than holding a /// single pushed scalar, so concurrent sessions add up instead of overwriting one another, /// and a disposed registration (a worker client going away) drops out of the sum. Disposal /// is idempotent because a client's dispose path can run twice. diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs index 8ae6601..21c2afe 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs @@ -20,7 +20,7 @@ public sealed class WorkerFrameReader private readonly WorkerFrameProtocolOptions _options; private readonly Stream _stream; - // Reused across frames rather than allocated per read (GWC-30). Safe because ReadAsync is + // Reused across frames rather than allocated per read (PERF-22). Safe because ReadAsync is // single-consumer by construction; the prefix is fully overwritten by every read. // // The single-consumer invariant has to survive teardown as well as steady state, because a read diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs index 1c1c90d..0593a15 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs @@ -668,10 +668,10 @@ public sealed class WorkerFrameWriter // Serialize once into a single pooled buffer that carries the 4-byte length prefix followed // by the payload, then issue one stream write. This avoids a second serialization pass, a // separate prefix array, a separate prefix write, and any per-frame heap allocation — the - // gateway-side writer's GWC-30 shape, now matched on the net48 side. The rented buffer may - // be larger than requested, so only the first frameLength bytes are ever written. The buffer - // is returned only after this frame's write has completed; the batch flush deferred to the - // end of the drain (see DrainQueuedFramesAsync) does not read from it. + // gateway-side writer's pooled-buffer shape (PERF-22), now matched on the net48 side. The + // rented buffer may be larger than requested, so only the first frameLength bytes are ever + // written. The buffer is returned only after this frame's write has completed; the batch + // flush deferred to the end of the drain (see DrainQueuedFramesAsync) does not read from it. int frameLength = sizeof(uint) + payloadLength; byte[] frame = ArrayPool.Shared.Rent(frameLength); try