diff --git a/docs/WorkerFrameProtocol.md b/docs/WorkerFrameProtocol.md index 3bab2a3..e2f25b9 100644 --- a/docs/WorkerFrameProtocol.md +++ b/docs/WorkerFrameProtocol.md @@ -136,7 +136,11 @@ cost stays bounded: a pure-event pass — the event hot path — still pays exactly one flush however many frames drain together, a run of control frames still pays one for the whole run (never one per heartbeat, the syscall-per-frame cost the coalescing removed), and only a pass that actually -mixes both classes pays a second. +mixes both classes pays a second. Mixed passes are not exotic: command replies +travel at Control priority alongside heartbeats and faults, so a session under +sustained command traffic concurrent with event streaming can hit them +routinely. The cost stays bounded either way — one extra flush per class +transition present in the pass, not per frame. One consequence of the boundary flush is worth stating: a control frame whose run has already been flushed and completed is out of the drain's diff --git a/docs/plans/2026-08-15-deferred-remediation.md b/docs/plans/2026-08-15-deferred-remediation.md index e9dd38e..e1cf33e 100644 --- a/docs/plans/2026-08-15-deferred-remediation.md +++ b/docs/plans/2026-08-15-deferred-remediation.md @@ -308,3 +308,67 @@ Push the branch to origin, then on windev (`ssh windev`, clone `C:\build\mxacces | 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`. diff --git a/docs/plans/2026-08-15-perf-review-remediation.md b/docs/plans/2026-08-15-perf-review-remediation.md index 5b20b1c..83a975e 100644 --- a/docs/plans/2026-08-15-perf-review-remediation.md +++ b/docs/plans/2026-08-15-perf-review-remediation.md @@ -619,6 +619,11 @@ Commit anything found: `docs: remediation plan doc sweep` | Event-path triple async-iterator flattening | LOW-rated; touches the most invariant-dense code in the gateway for two `MoveNextAsync` hops per event. Reconsider after Tasks 3/4 land and if profiling still shows it. | | `SessionEventDistributor._subscribers` `ConcurrentDictionary` → plain `Dictionary` (Task 25 / Task 3 review) | Every mutation is already inside `_lifecycleLock`, so the concurrent type buys nothing. Behavior-neutral refactor with no measurable win, proposed after the windev gate was already green — not worth re-running the verification matrix for. Comment cleanups from the same review landed; this swap did not. | +All six rows above were resolved on 2026-08-15 by the follow-up plan +[`docs/plans/2026-08-15-deferred-remediation.md`](2026-08-15-deferred-remediation.md) +(branch `perf/deferred-remediation`), which also fixed the pre-existing Windows +`SecretsStorePathGuardTests` failure. + ## Execution notes for the orchestrator - Branch: `git checkout -b perf/review-remediation` before Task 1. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs index e2bc100..04212ab 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs @@ -56,9 +56,9 @@ public delegate void SubscriberOverflowHandler(bool isOnlySubscriber, bool isInt /// producing an /// of already-mapped public /// s, given a . This is the -/// cleanest seam: it can pass -/// ct => session.ReadEventsAsync(ct).Select(mapper.MapEvent) (or a -/// channel reader's ReadAllAsync), while unit tests pass a plain +/// cleanest seam: production passes GatewaySession.MapWorkerEventsAsync, +/// which reads the worker event channel and maps each frame in one iterator +/// (inlining what used to be a read-then-Select chain), while unit tests pass a plain /// channel reader's ReadAllAsync with no real session. The pump owns the /// single consumption of this enumerable; fan-out happens on the public /// after mapping, mirroring today's @@ -751,9 +751,9 @@ public sealed class SessionEventDistributor : IAsyncDisposable } // Disconnect ONLY this subscriber: it is already out of the fan-out set (removed above), - // so complete its channel with the overflow fault. Its gRPC reader's MoveNextAsync then - // throws the SessionManagerException, which EventStreamService surfaces to the client - // exactly as the pre-epic per-RPC overflow did. The pump and every other subscriber are + // so complete its channel with the overflow fault. EventStreamService consumes the channel + // directly, so its next WaitToReadAsync throws the SessionManagerException, which it surfaces + // to the client exactly as the pre-epic per-RPC overflow did. The pump and every other subscriber are // untouched. This runs even when the handler above threw — the subscriber must never be // left attached with an un-completed channel. subscriber.Channel.Writer.TryComplete(new SessionManagerException( diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs index 61db4c7..d94b118 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs @@ -179,7 +179,9 @@ public sealed class WorkerFrameWriter /// remaining events are written, so a batch in flight does not delay a control frame's delivery. /// Every frame's "written and flushed before completion" contract is unchanged. An event batch that /// a control frame cuts into therefore pays one extra flush; an uninterrupted batch still pays - /// exactly one. + /// exactly one. Mixed passes are not exotic — command replies are Control priority too, so + /// sustained command traffic concurrent with event streaming can hit them routinely; the cost + /// stays bounded at one extra flush per class transition present in the pass. /// /// Envelopes to write, in order. /// Scheduling priority for the whole batch.