dohertj2/mxaccessgw
1
0
Fork 0
Code Issues 14 Pull Requests 2 Actions Packages Projects Releases Wiki Activity
Files
e1ff05c605da64b33a253b96bd574a15d32f8a4d
mxaccessgw/docs/plans/2026-08-15-deferred-remediation.md
T

36 KiB
Raw Blame History

Deferred-Findings Remediation Implementation Plan

For Claude: REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development to implement this plan task-by-task (Opus implementers per the user's instruction).

Goal: Resolve the six findings the 2026-08-15 perf-review remediation explicitly deferred (docs/plans/2026-08-15-perf-review-remediation.md:611-620) plus the pre-existing Windows-only SecretsStorePathGuardTests failure, so the deferred table empties and windev returns to a clean 1046/1046 gateway suite.

Architecture: Two phases. Phase A is gateway-side (net10, fully verifiable on macOS): the secrets-test fix, the distributor dictionary swap, event-path iterator flattening, and the dashboard in-process refactor that removes the Blazor pages' loopback SignalR hop while preserving the idle gate, mirror viewer gating, and clone-then-redact invariants. Phase B is worker-side (net48 x86, verified on windev over ssh): control-frame completion decoupling in the two-class frame writer, pipe-read teardown restructuring, and value-cache clone removal per the completed aliasing audit.

Tech Stack: .NET 10 / ASP.NET Core / Blazor Server / System.Threading.Channels (gateway); .NET Framework 4.8 x86 (worker); xUnit; windev CI clone C:\build\mxaccessgw-ci via ssh windev.

Branch: perf/deferred-remediation off local main (15f188e).

Ground rules for every implementer subagent

  • Shared working tree at /Users/dohertj2/Desktop/MxAccessGateway. NEVER run git stash, git reset, git clean, git checkout <sha/branch>, or any command that touches files outside your task's Files: list. Commit with explicit pathspecs only (git add <your files> && git commit).
  • Build/test lock: before dotnet build or dotnet test, acquire the lock with mkdir /private/tmp/claude-501/-Users-dohertj2-Desktop-MxAccessGateway/f36938ae-bbca-4245-b5c9-fac512d69e22/scratchpad/buildlock (retry loop with sleep until it succeeds); rmdir it in ALL exit paths.
  • TreatWarningsAsErrors=true, Nullable=enable repo-wide. Follow docs/style-guides/CSharpStyleGuide.md: file-scoped namespaces, sealed by default, Async suffix, MXAccess-aligned names.
  • Worker projects (ZB.MOM.WW.MxGateway.Worker*) are net48/x86 and DO NOT COMPILE on macOS. For Phase B tasks: edit carefully, self-review for net48 compatibility (target-typed new and file-scoped namespaces ARE valid — LangVersion=latest; but no Span-based BCL overloads, no IAsyncDisposable on BCL types, Channel comes from System.Threading.Channels package which the worker already references). Compilation and tests happen at the Task 13 windev gate.
  • Update affected docs in the same commit as the source (repo rule), except the dashboard design doc which Task 8 consolidates (deliberate, to avoid parallel edits to one file).
  • MXAccess parity: never synthesize events, never mutate an event already handed to the outbound queue or wire.

Phase A — gateway (macOS-verifiable)

Task 1: Windows-safe cleanup in SecretsStorePathGuardTests

Classification: small Estimated implement time: ~3 min Parallelizable with: Task 2, Task 3, Task 4

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs
  • Modify: docs/GatewayTesting.md (lines ~557-565, the "fails deterministically on Windows" note)

Why: CreateBuilder_AcceptsSecretsStoreOutsideContentRoot_AndCreatesIt (lines 84-105) fails deterministically on Windows: GatewayApplication.CreateBuilder migrates the secrets store through SecretsSqliteConnectionFactory (Pooling = true, WAL), disposal returns the connection to the Microsoft.Data.Sqlite pool with the native handle open, and the finally's Directory.Delete(directory, recursive: true) (line 103) hits a sharing violation. macOS passes only because Unix unlinks open files. The repo fixes this pattern twice already: TestSupport/../TempDatabaseDirectory.cs:57 and Configuration/PreHostSecretExpansionTests.cs:130-153.

Spec:

  1. In the failing test's finally, before Directory.Delete: call Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools(); and wrap the delete in try { ... } catch (IOException) { } catch (UnauthorizedAccessException) { } (best-effort, mirroring TempDatabaseDirectory.Dispose). Add a comment mirroring the one in PreHostSecretExpansionTests.cs:133-137 (WAL + pooling keeps the handle alive past dispose).
  2. Leave the rejection test alone (the guard means its file is never created).
  3. Update docs/GatewayTesting.md: replace the "subtract it from the expected pass count on Windows" paragraph with a short note that the test's cleanup now clears the SQLite pool first and the failure is fixed as of this branch.

Steps: edit → dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~SecretsStorePathGuardTests" (expect 2/2 on macOS; the real proof is the Task 13 windev gate) → commit fix(tests): clear the SQLite pool before deleting the secrets path-guard temp dir — Windows sharing violation.

Task 2: SessionEventDistributor _subscribers → plain Dictionary

Classification: small Estimated implement time: ~3 min Parallelizable with: Task 1, Task 3, Task 4

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs

Why: All five access sites (:365, :555, :771, :799, :813) are inside lock (_lifecycleLock); the lock-free hot path reads the copy-on-write _subscriberSnapshot array (:958, :302), never the dictionary. The concurrent type buys nothing. Audit confirmed no external/reflection access.

Spec: Change the field at :107 to Dictionary<long, Subscriber>; TryRemove(subscriber.Id, out _) at :799 becomes Remove(subscriber.Id). Reword the type remarks at :69-80, :111-123, and :298-300 where they name ConcurrentDictionary by design — the invariant to state is now: "the dictionary is only ever touched under _lifecycleLock; lock-free readers use _subscriberSnapshot."

Steps: edit → dotnet test ... --filter "FullyQualifiedName~SessionEventDistributorTests" (29 facts, expect all green) → commit refactor(sessions): _subscribers to plain Dictionary — every access is under _lifecycleLock.

Task 3: Merge the session event-source pass-through iterator

Classification: standard Estimated implement time: ~5 min Parallelizable with: Task 1, Task 2, Task 4

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs (MapWorkerEventsAsync ~:767-776, ReadEventsAsync ~:1517-1530)
  • Test: src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/GatewaySessionTests.cs (existing)

Why: The worker→distributor source chain nests three compiler-generated async iterators per event: WorkerClient.ReadEventsCoreAsync → GatewaySession.ReadEventsAsync (pure pass-through: TouchClientActivity(); yield return) → GatewaySession.MapWorkerEventsAsync (yield return mapper.MapEvent(...)). The pass-through layer is two extra MoveNextAsync state-machine hops per event for no semantic value.

Spec:

  1. FIRST grep all callers of ReadEventsAsync. If MapWorkerEventsAsync is its only caller, inline it: MapWorkerEventsAsync calls GetReadyWorkerClientAsync, iterates client.ReadEventsAsync(ct) directly, calls TouchClientActivity() per event, and yield return mapper.MapEvent(workerEvent). Delete ReadEventsAsync. If other callers exist, keep the method for them but make MapWorkerEventsAsync self-contained as above — do NOT change any caller outside this file; report the finding.
  2. Behavior must be byte-identical: same activity-touch cadence (per event), same exception propagation (WorkerClientException flows to the distributor pump unchanged), no event synthesis, worker order preserved.
  3. WorkerClient.ReadEventsCoreAsync's single-reader claim (_eventsReaderClaimed) must still be exercised exactly once per attach — do not add a second call site.

Steps: grep callers → edit → dotnet test ... --filter "FullyQualifiedName~GatewaySession" and --filter "FullyQualifiedName~SessionEventDistributorTests" → commit perf(sessions): fold the ReadEventsAsync pass-through into MapWorkerEventsAsync — one fewer iterator per event.

Task 4: EventStreamService direct channel reads in the live loop

Classification: high-risk Estimated implement time: ~5 min Parallelizable with: Task 1, Task 2, Task 3

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs
  • Test: src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs (existing 17 facts — must pass unchanged)

Why: The subscriber-side live loop materializes subscriber.Reader.ReadAllAsync(ct).GetAsyncEnumerator(ct) (:109-111) — a BCL async-iterator wrapper costing a state-machine hop per event on the hottest gateway path. Direct ChannelReader consumption (WaitToReadAsync + drain-with-TryRead) removes it.

Spec:

  1. Replace the enumerator with direct reads: while (await reader.WaitToReadAsync(ct)) { while (reader.TryRead(out MxEvent? mxEvent)) { ...existing per-event body... } }; loop ends when WaitToReadAsync returns false (channel completed).
  2. EVERY invariant in the current body survives, verbatim where possible:
    • ReplayGap sentinel emitted exactly once, first, only when replayGap (:133-139) — untouched, it precedes the live loop.
    • Replay batch stitching (:141-150) — untouched.
    • Per-RPC dedup watermark if (mxEvent.WorkerSequence <= afterWorkerSequence) continue; (:179-182) — must apply to every live event.
    • WorkerClientException catch → session.MarkFaulted → metrics → rethrow (:164-174): a completed-with-exception channel surfaces its exception from WaitToReadAsync — the catch must wrap the wait/read, preserving identical fault classification. Terminal SessionManagerException(EventQueueOverflow) propagates unchanged.
    • finally ordering (:192-200): with no enumerator to dispose, the remaining order is backlog-gauge registration disposal → lease disposal → metrics.StreamDisconnected("Detached"). Keep the comments explaining why.
  3. Cancellation: WaitToReadAsync(ct) throws OperationCanceledException on detach — must reach the same code path the enumerator's cancellation did (the gRPC layer treats it as client disconnect). Verify against StreamEventsAsync_WhenCanceled_DetachesSubscriber.
  4. No public-surface change; MxAccessGatewayService (:151-179) is untouched.

Steps: edit → run the full EventStreamServiceTests class + GatewayEndToEndReconnectReplayTests + GatewayEndToEndMultiSubscriberTests → commit perf(grpc): consume the subscriber channel directly in StreamEventsAsync — drops the ReadAllAsync iterator hop.

Task 5: In-process dashboard snapshot feed + page switch

Classification: high-risk Estimated implement time: ~8 min (accepted overage; splitting further would split one invariant) Parallelizable with: Task 6, Task 7

Files:

  • Create: src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSnapshotFeed.cs
  • Create: src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs
  • Modify: src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs
  • Modify: src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs (one AddSingleton line)
  • Create: src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs

Why: Eight pages inherit DashboardPageBase and each opens a loopback /hubs/snapshot HubConnection (DashboardPageBase.cs:62) — a WebSocket round trip back into the same process per circuit. IDashboardSnapshotService.WatchSnapshotsAsync exists but is NOT multicast (each enumeration = its own PeriodicTimer + snapshot build), so pages must not call it directly; a shared feed does one enumeration and fans out.

Spec:

  1. IDashboardSnapshotFeed (singleton): IAsyncEnumerable<DashboardSnapshot> WatchAsync(CancellationToken ct). Internally: per-subscriber Channel<DashboardSnapshot> with capacity 1 and BoundedChannelFullMode.DropOldest (a dashboard viewer only ever wants the latest snapshot; a slow circuit must never buffer unboundedly or stall others).
  2. Idle gating (the invariant this task must not lose): the feed enumerates IDashboardSnapshotService.WatchSnapshotsAsync on a background task started when the subscriber count goes 0→1 and cancelled when it goes 1→0. While zero subscribers, the feed holds no timer and builds no snapshot. Guard subscriber add/remove with a plain lock; restart cleanly on resubscribe (mirror the start/stop discipline of GatewayAlarmMonitor.StreamAsync registration, GatewayAlarmMonitor.cs:739-752). If the underlying watch throws or completes, complete all subscriber channels with the error and reset so the next subscriber restarts it (mirror DashboardSnapshotPublisher.ExecuteAsync's reconnect-after-delay posture, but per-feed).
  3. DashboardPageBase: remove the HubConnection path (:62 and the factory usage); keep the synchronous first render via snapshotService.GetSnapshot() (:37); then a background loop await foreach (var s in feed.WatchAsync(_cts.Token)) { Snapshot = s; await InvokeAsync(StateHasChanged); } started in OnAfterRenderAsync(firstRender) or OnInitializedAsync (match current lifecycle), cancelled + awaited in DisposeAsync. Update the class XML doc that narrates the hub subscription history (:7-14).
  4. Hubs, DashboardSnapshotPublisher, DashboardSnapshotHubConnectionCounter, DashboardHubConnectionFactory, and /hubs/token all stay — they remain the remote/external surface. Do not touch them.
  5. Auth: the pages are mapped behind ViewerPolicy (DashboardEndpointRouteBuilderExtensions.cs:136), which remains the gate for in-process consumption; add one comment on WatchAsync saying so.
  6. Tests (DashboardSnapshotFeedTests): (a) zero subscribers → underlying service's WatchSnapshotsAsync never enumerated (fake service counts enumerations/MoveNextAsync); (b) first subscriber starts exactly one enumeration; two subscribers share it; (c) last unsubscribe cancels it; resubscribe restarts it; (d) slow subscriber observes latest-wins (push 3 snapshots, read 1, it is the newest) while a fast subscriber sees all; (e) underlying fault completes subscribers with the error and a fresh subscriber restarts.

Steps: write feed tests first (fail) → implement feed → page switch → dotnet test ... --filter "FullyQualifiedName~DashboardSnapshotFeed" then --filter "FullyQualifiedName~Dashboard" (whole dashboard test folder) → commit feat(dashboard): in-process snapshot feed replaces the pages' loopback /hubs/snapshot hop.

Task 6: In-process session event subscription + SessionDetailsPage switch

Classification: high-risk Estimated implement time: ~8 min Parallelizable with: Task 5, Task 7

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs
  • Modify: src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHubViewerRegistry.cs (only if a member is needed for synthetic connection ids; prefer reusing the existing API)
  • Modify: src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor (the /hubs/events connection at :271,297)
  • Test: src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardEventBroadcasterTests.cs (extend)

Why: SessionDetailsPage opens a loopback /hubs/events connection. The broadcaster already short-circuits on !viewerRegistry.HasViewers(sessionId) BEFORE the redaction deep clone (DashboardEventBroadcaster.cs:51-56) — the mirror viewer gating shipped last round. An in-process subscription must keep feeding that registry or every unwatched session pays MxEvent.Clone() per event again.

Spec:

  1. Add to DashboardEventBroadcaster an in-process subscribe API: IDashboardEventSubscription Subscribe(string sessionId) returning a disposable that exposes ChannelReader<MxEvent> Reader (bounded, capacity ~256, DropOldest — this is a UI mirror, loss is acceptable and already documented for the hub path). On subscribe: register a synthetic connection id (e.g. "inproc-" + Guid.NewGuid().ToString("N")) with EventsHubViewerRegistry.AddViewer(connectionId, sessionId); on dispose: RemoveViewer + ReleaseConnection in the order the hub uses (EventsHub.cs:86,99). Registry stays the single source of truth for HasViewers.
  2. Publish (:39-86): after the existing HasViewers check and the clone-then-redact (RedactValues :97-109), TryWrite the SAME redacted clone to each in-process subscriber of that session, in addition to the hub group send. The source MxEvent is shared with the gRPC stream and replay ring — the existing never-mutate-in-place rule holds; in-process subscribers receive the redacted clone only.
  3. SessionDetailsPage: replace the HubConnection + SubscribeSession invoke with broadcaster.Subscribe(SessionId) and a read loop marshalling to the renderer via InvokeAsync(StateHasChanged); dispose the subscription in DisposeAsync. Keep the existing per-session ACL posture (any Viewer may watch any session — SEC-25 is tracked separately; do not widen or narrow it here).
  4. Tests to add in DashboardEventBroadcasterTests: (a) in-process subscriber receives the redacted event when ShowTagValues=false and the source event is not mutated; (b) subscribing flips HasViewers so Publish stops short-circuiting (proves mirror gating integration); (c) disposing the last in-process subscriber restores the no-viewers short-circuit (no clone, no send — reuse the existing Publish_WithNoRegisteredViewers_DoesNotCloneOrSend fake pattern); (d) hub viewers and in-process viewers are independently counted.

Steps: tests first → implement → dotnet test ... --filter "FullyQualifiedName~DashboardEventBroadcaster" + --filter "FullyQualifiedName~EventsHubViewerRegistry" + --filter "FullyQualifiedName~GatewaySessionDashboardMirror" → commit feat(dashboard): in-process session event subscription feeds the viewer registry — SessionDetailsPage drops its /hubs/events hop.

Task 7: AlarmsPage provider-status via IGatewayAlarmService

Classification: standard Estimated implement time: ~4 min Parallelizable with: Task 5, Task 6

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/AlarmsPage.razor (:194 HubConnection, :281-304 poll loop untouched)

Why: AlarmsPage opens /hubs/alarms but only consumes ProviderStatus payloads from it (alarm rows come from the 3 s QueryAlarmsAsync poll). IGatewayAlarmService.StreamAsync (GatewayAlarmMonitor.cs:724-777) is already a true multi-subscriber in-process fan-out.

Spec: Replace the HubConnection with a background loop over alarmService.StreamAsync(alarmFilterPrefix: null, ct), handling only PayloadOneofCase.ProviderStatus (skip snapshot/live alarm payloads — the poll stays authoritative for rows). The monitor's drop policy completes a lagging subscriber's channel (:700-712): on completion or fault, delay ~1 s and resubscribe (matching the hub path's WithAutomaticReconnect posture). Dispose via the page's existing cancellation. Leave the poll loop alone.

Steps: edit → dotnet build src/ZB.MOM.WW.MxGateway.Server → dotnet test ... --filter "FullyQualifiedName~Alarms" → commit feat(dashboard): AlarmsPage reads provider status from IGatewayAlarmService in-process.

Task 8: Dashboard design-doc update (consolidated)

Classification: small Estimated implement time: ~4 min Parallelizable with: none (runs after 5, 6, 7 land)

Files:

  • Modify: docs/GatewayDashboardDesign.md (sections at ~:112-114, :162-178, :190-217, :228-247, :535-541, :581-595)

Spec: Rewrite the affected sections to describe: pages consume in-process seams (IDashboardSnapshotFeed, DashboardEventBroadcaster.Subscribe, IGatewayAlarmService.StreamAsync); the three hubs and /hubs/token remain as the remote/external surface; idle gating is now two-tier (hub connection counter gates the hub publisher; feed subscriber count gates the in-process pump — while nobody watches, neither builds a snapshot); mirror gating counts hub viewers AND in-process viewers through the one registry; clone-then-redact still happens once in the broadcaster before any delivery; ViewerPolicy on the component endpoint is the in-process auth gate; SEC-25 per-session ACL gap unchanged. Present tense, why-not-what, no marketing.

Commit: docs(dashboard): in-process page feeds, two-tier idle gating, hubs as the external surface

Task 9: Phase A gate — full gateway suite on macOS

Classification: trivial (verification only) Parallelizable with: none (after Tasks 1-8)

Run dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx (expect 0 warnings) and the full dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj (expect ≥1046 passed, 0 failed; new feed/broadcaster tests raise the count). Fix-forward any failure before Phase B.

Phase B — worker (net48 x86, verified on windev)

Task 10: Control-frame completion decoupling in WorkerFrameWriter

Classification: high-risk Estimated implement time: ~6 min Parallelizable with: Task 11, Task 12

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs
  • Modify: src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs
  • Modify: docs/WorkerFrameProtocol.md (~:120-131 completion-semantics paragraph)

Why: Wire ordering is already correct — DequeueNext (:383-413) re-checks _controlFrames before every frame. The coupling is completion latency: DrainQueuedFramesAsync (:304-361) defers the single FlushAsync and ALL TrySetResult calls to after the whole drain pass, so a heartbeat/command-reply/fault/shutdown-ack Task awaited by its writer does not resolve until up to 128 event frames behind it are written and flushed. The XML docs claim "never delayed behind an event backlog" — true of bytes, false of the awaited completion.

Spec:

  1. Record the priority class on PendingFrame (:23-48), set at construction in WriteAsync (:109) and WriteBatchAsync (:192).
  2. In DrainQueuedFramesAsync: when DequeueNext returns an Event frame while written contains one or more not-yet-completed Control frames, first FlushAsync + complete + clear written, then continue draining. Exit-path flush at :339-360 unchanged. Net effect: a control frame's completion never waits on an event frame dequeued after it; the pure-event 128-batch hot path still pays exactly one flush (guarded by the existing WriteAsync_WhenBatchDrainedTogether_FlushesOnce and EventBurst_DrainLoopCoalescesFlushes); a pure-control burst still pays one flush. Do NOT flush per control frame unconditionally — that reintroduces the pre-WRK-12 syscall-per-heartbeat cost.
  3. Failure handling: FailFrames(written, ...) / FailAllQueued (:327-336) operate on the current written list; after an early flush+complete+clear, frames already completed must not be failable — verify the clear ordering makes that structurally true, and extend the fault-injection tests if the early-flush path adds a new failure window (a FlushAsync fault with a partially-completed pass).
  4. New test (use the existing GatedWriteStream harness ~:880): queue a control frame behind N gated event frames within one drain pass; assert the control frame's WriteAsync task completes before the last event write is released. Keep all 9 existing writer tests green — sequence stamping (:431-483), claim/tombstone interlock (:244-274), and wire order must be untouched.
  5. docs/WorkerFrameProtocol.md: update the completion-semantics paragraph — completion now resolves at the class-transition flush, still meaning "written AND flushed".

Commit: perf(worker): control-frame completions resolve at the class-transition flush, not after the event batch

Task 11: Worker pipe-read teardown — dispose-to-unblock and observe the abandoned read

Classification: high-risk Estimated implement time: ~8 min Parallelizable with: Task 10, Task 12

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs (RunMessageLoopAsync :267-310, ctor :55-68)
  • Modify: src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeClient.cs (:143-159) — only if ownership must move; prefer not
  • 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.

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.
  2. Ordering: final writes complete before disposal. The shutdown ack (WriteShutdownAckAsync :1064-1069) and fault frames (TryWriteFaultAsync :1164+) are written after the message loop exits on some paths — trace every exit path and place the stream disposal AFTER the last possible write on each. The clean design: WorkerPipeSession keeps a reference to the ctor Stream; RunAsync's outermost finally (after runtime-session disposal and any fault write, :133-145) disposes the stream and then observes readTask (stored in a field by RunMessageLoopAsync). WorkerPipeClient's using then double-disposes harmlessly. If the trace shows a fault write that happens in WorkerPipeClient after session.RunAsync returns (there is none known), fall back to moving observation into WorkerPipeClient.
  3. Never a second read. After abandonment, no code path may call _reader.ReadAsync again (pooled-buffer use-after-return). The message loop already guarantees this (return before reassignment on the graceful path); keep it structurally true and assert it in a comment on _lengthPrefix (WorkerFrameReader.cs:23-25).
  4. Graceful path unchanged: WorkerShutdown/ShutdownWorker exits have no pending read; disposal+observation must be a no-op there (observe a completed/absent task).
  5. Document the net48 token-ignoring fact where the read is issued (RunMessageLoopAsync and/or ReadExactlyOrThrowAsync) — the research found zero comments acknowledging it.
  6. Tests (net48 project, real PipePair harness :2433-2485): (a) fault-path exit (reuse the RunAsync_EventFrameTooLarge_... shape :868) — assert RunAsync completes within the existing 5 s bound AND, via a TaskScheduler.UnobservedTaskException hook armed in the test with a forced GC, that no unobserved exception leaks; (b) graceful shutdown still completes with no pending read; (c) the session disposes the stream (harness observes the gateway-side stream faulting its own pending read promptly rather than at PipePair.Dispose).

Commit: fix(worker): session-owned stream disposal unblocks and observes the net48 pipe read at teardown

Task 12: Value-cache clone removal per the aliasing audit

Classification: standard Estimated implement time: ~5 min Parallelizable with: Task 10, Task 11

Files:

  • Modify: src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessValueCache.cs (Set :82,83,97; CachedValue :275)
  • Modify: src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessValueCacheTests.cs (rewrite the :58 test)
  • Modify: src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessCommandExecutorTests.cs (add cached-read test)

Why (audit result): All three clones in Set — Value.Clone() (deep, recursive for arrays), SourceTimestamp.Clone(), Statuses.Clone() (container + N proxies) — are removable. The event is fully stamped BEFORE Set runs (Enqueue at MxAccessBaseEventSink.cs:263 precedes postPublish at :288; sequence/timestamp stamped inside Enqueue, MxAccessEventQueue.cs:269-270) and the queue's ownership invariant forbids later mutation. The alias already exists on the read side: SucceededRead (MxAccessSession.cs:1086,1091,1096) hands the cache's own Value/SourceTimestamp instances into every BulkReadResult, which downstream only wraps and serializes. Worker↔gateway is a process boundary — no gateway consumer can alias.

Spec:

  1. Remove all three clones; CachedValue stores the event's own references.
  2. Ownership contract comment on Set and on CachedValue: the cache holds borrowed references into an enqueued, write-once MxEvent; consumers may read and serialize, never mutate; mutation would additionally invalidate QueuedEvent.Size — the enqueue-time memoized serialized size that the byte-budgeted Drain charges (MxAccessEventQueue.cs:499-506), so a grown message could overshoot the negotiated frame max and fault the session via MessageTooLarge.
  3. Rewrite Set_StoresIndependentSnapshot_UnaffectedByLaterEventMutation (:58 — it codifies the invariant being reversed) into the aliasing contract: Set then TryGet returns the same Value/SourceTimestamp/Statuses-element instances (Assert.Same), with the doc comment explaining the write-once borrow.
  4. Add the missing cached-read-path test in MxAccessCommandExecutorTests: seed the cache, dispatch a ReadBulk that hits TryGetCachedReadFor → assert WasCached == true and result.Value is reference-equal to the cached instance (closing the coverage gap the audit found — nothing today exercises WasCached == true end-to-end in the worker).
  5. MxAccessWriteCompletionCache.Record's parallel statuses.Clone() (:76) is left AS-IS deliberately (different lifecycle, not in the finding) — add one cross-reference comment there pointing at the value-cache ownership contract.

Commit: perf(worker): value cache borrows the write-once event's instances — three clones per OnDataChange removed

Task 13: Phase B gate — windev full verification

Classification: trivial (verification only) Parallelizable with: none (after Tasks 10-12; Phase A gate must be green)

Push the branch to origin, then on windev (ssh windev, clone C:\build\mxaccessgw-ci): fetch + checkout the branch; dotnet build src/ZB.MOM.WW.MxGateway.slnx (0 warnings); dotnet build src/ZB.MOM.WW.MxGateway.Worker/... -p:Platform=x86; dotnet test src/ZB.MOM.WW.MxGateway.Worker.Tests/... -p:Platform=x86 (expect 501+ passed, 0 failed — new tests raise the count); dotnet test src/ZB.MOM.WW.MxGateway.Tests/... (expect 0 failed including SecretsStorePathGuardTests — the Task 1 proof). Known caveat: the reconnect-replay test is load-sensitive on windev; re-run isolated before treating it as a regression (documented in docs/GatewayTesting.md).

Task 14: Wrap-up — deferred table closure, docs sweep, final review

Classification: small Parallelizable with: none (last)

  • Append a closure note to docs/plans/2026-08-15-perf-review-remediation.md's deferred table (one line: resolved by this plan, date, branch).
  • Sweep: gateway.md / docs/WorkerFrameProtocol.md / docs/GatewayDashboardDesign.md / docs/GatewayTesting.md consistency with as-built behavior; record any accepted deviations in THIS plan's "As-built notes" section (add it).
  • Update .tasks.json statuses; update auto-memory (perf-remediation-branch.md or successor) with the branch state.
  • Dispatch the final integration code review (Opus) over git diff main..perf/deferred-remediation before reporting done. Merge remains the user's decision.

Explicitly out of scope

Item Why
wnwrap alarm GUID identity semantics; ALARM_RECORDS/@COUNT probe Need live alarms on windev — external state this plan cannot provide. Still tracked in the prior plan's follow-ups.
Structural alarm-truncation degraded-status signal Contract-level design (proto change candidate) — separate effort.
SEC-25 per-session dashboard event ACL Security roadmap item; Task 6 deliberately preserves the current posture.
MxAccessWriteCompletionCache clone Different lifecycle than the value cache; consciously kept (Task 12.5).

As-built notes (execution record)

Where the delivered work differs from the task text above, or where the route to it is worth keeping, this is the record.

Task 3 — ReadEventsAsync retained. The method was not removed after MapWorkerEventsAsync inlined the read-then-map chain: a second caller reaches it through ISessionManager.ReadEventsAsync. That interface member itself has zero production call sites — only test fakes implement and exercise it. Deleting it is a mechanical but wide change (~15 test-fake touches), so it is recorded as a follow-up rather than done here.

Task 5 — dashboard event feed, two review rounds. Review caught two races that the first cut did not have. First, subscription lifetime: subscriptions are now generation-tagged, a generation ends at the time the fault is observed (not when it is raised), Reset is scoped to the dying generation so it cannot cancel its successor, and a backstop restart covers the case where no subscriber is left to drive recovery. Second, UnsubscribeAsync needed a generation-scoped idle gate so a teardown for an old generation cannot tear down the new one. Both fixes are pinned by tests verified against mutations of the fixed code.

Task 6 — subscribe API placement. The subscribe surface lives on IDashboardSessionEventSubscriber, with DI forwarding to a single instance so every consumer shares one feed. Batches that arrive for a session the renderer has already moved off are dropped by a subscription identity check inside the renderer dispatch, which is what makes a stale batch harmless rather than a cross-session leak.

Task 10 — the delivered property is the delivery point, not awaited latency. The spec asked for control-frame completion to be observable before the pass's event writes. That is unachievable in the enqueue-then-contend shape: a caller that loses the write-lock race does not run again until the winning drainer releases the lock, so its await cannot return early no matter when its frame completes. What shipped is the honest half: control frames are written and flushed at the class-transition boundary, so the priority class governs the frame's delivery point rather than only its byte order. Getting the awaited-latency win too requires unparking the lock-race loser from the winner's pass — a change to the write-lock shape, recorded as a follow-up. One extra FlushFileBuffers per mixed pass is the accepted cost.

Task 11 — teardown ordering and unconditional fault observation. Teardown disposes the session-owned transport first, then observes the read that dispose abandoned. Fault observation is unconditional — a ContinueWith(..., TaskContinuationOptions.OnlyOnFaulted) continuation, so the budget that bounds the wait is diagnostics-only and can never be the reason a fault goes unobserved. The same continuation covers heartbeat and drain overrun. DisposeTransportStream is exception-total: no dispose path can throw out of teardown.

Task 12 — three clones removed, plan rationale corrected in-code. All three OnDataChange value-cache clones are gone. The plan's stated reason for keeping the MxAccessWriteCompletionCache clone ("different lifecycle than the value cache") is wrong and was corrected where the code documents it: the clone is kept on provenance grounds — the cached payload comes from a caller-supplied object the worker does not own — not on lifecycle grounds.

Task 13 — windev verification. Solution build 0 warnings / 0 errors after clearing stale Contracts obj artifacts (an infrastructure problem on the box, not a regression from this branch). Worker x86: 509/509 (+8 new). Gateway: 1059/1059, including SecretsStorePathGuardTests — the first fully green Windows gateway run, that suite having been red before this branch. One load flake (InvokeAsync_WhenWorkerHandshakingThenReadyWithinTimeout_Succeeds) passed in isolation and on re-run, consistent with the load-sensitivity caveat documented in docs/GatewayTesting.md.

Reference in New Issue View Git Blame Copy Permalink