Merge fix/c3-tracker-hygiene: GWC-31 perf-pass comments relabeled PERF-nn, archreview prefixes reserved in REVIEW-PROCESS.md, cycle-1/2 tracker housekeeping (GWC-16 Won't fix, IPC-08/IPC-31 Done, TST-06/17 refreshed, WRK-29..31 reserved)
ci / nightly-windev (push) Has been skipped
ci / windows-x86 (push) Successful in 1m57s
ci / java (push) Successful in 2m11s
ci / portable (push) Successful in 9m23s

This commit is contained in:
Joseph Doherty
2026-08-20 09:40:05 -04:00
18 changed files with 65 additions and 27 deletions
+6
View File
@@ -79,6 +79,12 @@ Add one entry per finding to the `## Findings` section of the module's
- **Finding ID** — `<Module>-NNN`, numbered sequentially within the module and - **Finding ID** — `<Module>-NNN`, numbered sequentially within the module and
never reused (e.g. `Worker-001`). IDs are permanent even after resolution. 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:** - **Severity:**
- **Critical** — data loss, security breach, crash/deadlock, or outage. - **Critical** — data loss, security breach, crash/deadlock, or outage.
- **High** — incorrect behaviour with significant impact; no safe workaround. - **High** — incorrect behaviour with significant impact; no safe workaround.
@@ -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-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-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-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) ### 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-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-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-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 | | 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) ### Security & dashboard — [40-security-dashboard.md](40-security-dashboard.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-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-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-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. |
--- ---
@@ -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. **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) ## IPC-32 — `check-codegen.ps1` check labels are miscounted `Info` · `—` (rides IPC-25)
@@ -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 | Done | | A | `fix/c3-client-versions` | CLI-46 | sonnet | Mechanical five-client bump + README note + one script guard; recipe is fully specified | Done |
| 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 | Done | | 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 | Done |
| 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 (`3619152`) |
| 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 | **Done** (`b177636` GWC-38, `2ef7b93` SEC-41+SEC-40, `8257a03` comment fixup; branch not merged/pushed) | | 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 | **Done** (`b177636` GWC-38, `2ef7b93` SEC-41+SEC-40, `8257a03` comment fixup; branch not merged/pushed) |
| 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 | **Done** (`577a0db` IPC-33, `a71663a` IPC-39, `b27035b` IPC-34, `ab41362` IPC-35; branch `fix/c3-ipc-boundaries`, not merged/pushed) — worker half pending windev (`WorkerPipeSession.ResolveDrainReplyByteBudget` and the `WorkerPipeSessionTests` floor-test comment cannot be built on macOS) | | 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 | **Done** (`577a0db` IPC-33, `a71663a` IPC-39, `b27035b` IPC-34, `ab41362` IPC-35; branch `fix/c3-ipc-boundaries`, not merged/pushed) — worker half pending windev (`WorkerPipeSession.ResolveDrainReplyByteBudget` and the `WorkerPipeSessionTests` floor-test comment cannot be built on macOS) |
| 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 | Done-pending-windev | | 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 | Done-pending-windev |
+4 -4
View File
@@ -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-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-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-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-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-18 | Low | — | S | — | Not started | `GatewaySession` implements `DisposeAsync` without `IAsyncDisposable` |
| GWC-19 | Low | — | S | — | Not started | Dead `GatewaySession.KillWorker(string)` + stale doc | | 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-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-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-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-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-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 | | 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-03 | High | P1 | M | — | Done | No CI exists |
| TST-04 | High | P2 | L | — | Done | Session-resilience epic 16/28 tasks unfinished | | 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-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-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-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 | | 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-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-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-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-18 | Low | — | S | — | Not started | Hosted-service wrappers untested |
| TST-19 | Low | — | S | — | Not started | Keep FakeWorkerHarness canned replies in lockstep | | 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 | | TST-20 | Low | — | S | — | Not started | E2E script papers over a real advise-without-consumer sharp edge |
@@ -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"`. **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` · `—` ## GWC-17 — Sessions layer throws `Grpc.Core.RpcException` `Low` · `—`
@@ -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`. **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` ## IPC-09 — Known codegen fragilities have no in-repo guards `Medium` · `P1`
@@ -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`. **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` · `—` ## 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). **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` · `—` ## TST-18 — Hosted-service wrappers untested `Low` · `—`
+2 -2
View File
@@ -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.sessions.open` | `_openSessions` | Currently open sessions tracked by `SessionManager`. |
| `mxgateway.workers.running` | `_workersRunning` | Worker clients in a running state. | | `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. | | `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 ## 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. - `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`. - `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 ## Recording Sites
@@ -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.Tests/Ipc/WorkerPipeSessionTests.cs`
- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs` (comment only) - 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:** **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. 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.
@@ -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) ## Explicitly deferred (decided, not forgotten)
| Finding | Why deferred | | Finding | Why deferred |
@@ -43,7 +43,7 @@ public sealed class GatewayMetrics : IDisposable
private readonly ConcurrentDictionary<long, Func<int>> _eventStreamBacklogSources = new(); private readonly ConcurrentDictionary<long, Func<int>> _eventStreamBacklogSources = new();
private long _nextEventStreamBacklogSourceId; 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 // 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 // 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 // arbitrary session's backlog instead of the gateway's. Each client registers a source returning
@@ -210,7 +210,7 @@ public sealed class GatewayMetrics : IDisposable
/// <param name="method">Name of the command method.</param> /// <param name="method">Name of the command method.</param>
public void CommandStarted(string 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 // Interlocked rather than _syncRoot — the same idiom as EventReceived. Nothing here needs a
// consistent multi-field view; GetSnapshot reads each with Interlocked.Read. // consistent multi-field view; GetSnapshot reads each with Interlocked.Read.
Interlocked.Increment(ref _commandsStarted); 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 /// Registers a live depth source for the worker event queue-depth gauge and returns a handle
/// that removes it when disposed. Each <c>WorkerClient</c> registers once and reports its own /// that removes it when disposed. Each <c>WorkerClient</c> registers once and reports its own
/// undelivered (staged + queued) event count, so the gauge is the gateway-wide sum instead of /// 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).
/// </summary> /// </summary>
/// <param name="depth"> /// <param name="depth">
/// Returns this worker client's current undelivered event count. Invoked only at collection /// 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' // 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 // 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 workerEventQueueDepth = GetWorkerEventQueueDepth();
int grpcEventStreamQueueDepth = GetGrpcEventStreamQueueDepth(); int grpcEventStreamQueueDepth = GetGrpcEventStreamQueueDepth();
lock (_syncRoot) 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); private int GetWorkerEventQueueDepth() => SumSources(_workerEventQueueDepthSources);
// Sums the live backlog across every registered event-stream subscriber. // Sums the live backlog across every registered event-stream subscriber.
@@ -41,7 +41,7 @@ public sealed class WorkerClient : IWorkerClient
private readonly SemaphoreSlim _pendingCommandSlots; private readonly SemaphoreSlim _pendingCommandSlots;
private readonly CancellationTokenSource _stopCts = new(); 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 // 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 // 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. // 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 // size the caller controls; checking here keeps a MessageTooLarge in the write loop a
// genuine desync signal. // 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 // 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 // (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 // is a lower bound on the frame the writer actually emits, never the frame length. Passing
@@ -257,7 +257,7 @@ public sealed class WorkerClient : IWorkerClient
await EnqueueAsync(commandEnvelope, cancellationToken).ConfigureAwait(false); 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 // 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 // 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 // continuation — the old shape allocated a linked CancellationTokenSource, its
@@ -354,7 +354,7 @@ public sealed class WorkerClient : IWorkerClient
{ {
await foreach (WorkerEvent workerEvent in _events.Reader.ReadAllAsync(cancellationToken).ConfigureAwait(false)) 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); Interlocked.Decrement(ref _eventQueueDepth);
yield return workerEvent; yield return workerEvent;
} }
@@ -164,7 +164,7 @@ public sealed class WorkerClientTests
TestTimeout, TestTimeout,
CancellationToken.None); 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 // ahead of the second command on the FIFO pipe; skip it rather than mistaking it for the
// command this assertion is about. // command this assertion is about.
WorkerEnvelope secondCommand = await ReadNextCommandAsync(pipePair, timedOutCommand.CorrelationId); WorkerEnvelope secondCommand = await ReadNextCommandAsync(pipePair, timedOutCommand.CorrelationId);
@@ -180,7 +180,7 @@ public sealed class WorkerClientTests
/// <summary> /// <summary>
/// A command timeout abandons the gateway-side wait, but the worker keeps the correlation on /// 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 <c>WorkerCancel</c> /// its single STA queue and would still run it — so the gateway forwards a <c>WorkerCancel</c>
/// 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 /// 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. /// cancel is protocol behavior the worker depends on, not an internal detail.
/// </summary> /// </summary>
@@ -19,7 +19,7 @@ public sealed class GatewayMetricsTests
metrics.CommandFailed("WriteSecured", "AuthorizationFailed", TimeSpan.FromMilliseconds(12)); metrics.CommandFailed("WriteSecured", "AuthorizationFailed", TimeSpan.FromMilliseconds(12));
metrics.EventReceived("session-1", "OnDataChange"); metrics.EventReceived("session-1", "OnDataChange");
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. // registrations below stand in for two concurrent sessions holding 3 and 4 events.
using IDisposable workerDepthSourceA = metrics.RegisterWorkerEventQueueDepthSource(static () => 3); using IDisposable workerDepthSourceA = metrics.RegisterWorkerEventQueueDepthSource(static () => 3);
using IDisposable workerDepthSourceB = metrics.RegisterWorkerEventQueueDepthSource(static () => 4); using IDisposable workerDepthSourceB = metrics.RegisterWorkerEventQueueDepthSource(static () => 4);
@@ -58,7 +58,7 @@ public sealed class GatewayMetricsTests
} }
/// <summary> /// <summary>
/// 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, /// 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 /// 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. /// is idempotent because a client's dispose path can run twice.
@@ -20,7 +20,7 @@ public sealed class WorkerFrameReader
private readonly WorkerFrameProtocolOptions _options; private readonly WorkerFrameProtocolOptions _options;
private readonly Stream _stream; 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. // 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 // The single-consumer invariant has to survive teardown as well as steady state, because a read
@@ -668,10 +668,10 @@ public sealed class WorkerFrameWriter
// Serialize once into a single pooled buffer that carries the 4-byte length prefix followed // 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 // 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 // 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 // gateway-side writer's pooled-buffer shape (PERF-22), now matched on the net48 side. The
// be larger than requested, so only the first frameLength bytes are ever written. The buffer // rented buffer may be larger than requested, so only the first frameLength bytes are ever
// is returned only after this frame's write has completed; the batch flush deferred to the // written. The buffer is returned only after this frame's write has completed; the batch
// end of the drain (see DrainQueuedFramesAsync) does not read from it. // flush deferred to the end of the drain (see DrainQueuedFramesAsync) does not read from it.
int frameLength = sizeof(uint) + payloadLength; int frameLength = sizeof(uint) + payloadLength;
byte[] frame = ArrayPool<byte>.Shared.Rent(frameLength); byte[] frame = ArrayPool<byte>.Shared.Rent(frameLength);
try try