Files
mxaccessgw/archreview/20-worker.md
T
Joseph Doherty 59856b8c63 docs(archreview): add architecture review + per-domain remediation designs and tracker
Adds the 2026-07-08 architecture review (00-overall + six domain reports)
and a remediation/ tree: one design+implementation doc per domain covering
every finding, plus 00-tracking.md as the master progress tracker.

- 153 findings with stable IDs (GWC/WRK/IPC/SEC/CLI/TST), each with
  design rationale, implementation steps, tests, docs, and verification.
- Tracker rolls findings up by severity and P0/P1/P2 roadmap tier, records
  cross-cutting clusters and per-finding status (all Not started).
- Planning docs only; no source changes.
2026-07-09 00:39:00 -04:00

139 lines
23 KiB
Markdown

# Worker Process — Architecture Review
## Scope & method
This review covers `src/ZB.MOM.WW.MxGateway.Worker` (.NET Framework 4.8, x86): bootstrap (`Bootstrap/`), pipe IPC (`Ipc/`), the STA runtime (`Sta/`), the MXAccess session/command/event layer (`MxAccess/`), and the conversion layer (`Conversion/`). Every finding cites file and line evidence read directly from the working tree on 2026-07-08 (branch `feat/jdk17-client-retarget`). The review is static only — the worker builds exclusively on the Windows x86 host, so no build or test run was performed. `src/ZB.MOM.WW.MxGateway.Worker.Tests` was skimmed for coverage. Reference documents: `gateway.md`, `docs/MxAccessWorkerInstanceDesign.md`, `docs/WorkerSta.md`, `docs/WorkerFrameProtocol.md`, `docs/WorkerConversion.md`.
## Executive summary
- The STA core is correct: `StaRuntime.ThreadMain` runs the canonical wait/pump/dispatch loop over `MsgWaitForMultipleObjectsEx(QS_ALLINPUT, MWMO_INPUTAVAILABLE)` plus a `PeekMessage`/`TranslateMessage`/`DispatchMessage` drain, with an `AutoResetEvent` wake and a 50 ms idle pump interval (`Sta/StaRuntime.cs:245-251`, `Sta/StaMessagePump.cs:31-59`). This satisfies the architecture's hardest requirement.
- COM lifetime discipline is strong throughout: `Marshal.FinalReleaseComObject` on every teardown path, event-sink detach before release, cleanup in MXAccess handle order (UnAdvise → RemoveItem → Unregister), and the wnwrap/subtag alarm consumers release their own RCWs (`MxAccess/MxAccessSession.cs:1250-1288`, `MxAccess/WnWrapAlarmConsumer.cs:554-580`, `MxAccess/LmxSubtagAlarmSource.cs:203-251`).
- The most serious defect is a watchdog/`ReadBulk` interaction: a long-running STA command has no way to refresh `LastActivityUtc`, so a legitimate `ReadBulk` over enough uncached tags exceeds the 75-second `HeartbeatStuckCeiling` and self-faults as `StaHung`, after which every completed reply is dropped.
- An STA thread that dies after startup (for example a message-pump wait failure) is silent: the captured exception is never logged or reported, and every subsequent `InvokeAsync` task hangs forever.
- Commands arriving after graceful shutdown starts are dropped with no reply at all; the gateway's correlation wait can only time out.
- Envelope `sequence` is assigned before the writer lock is taken, so concurrently written frames can appear on the wire out of sequence order, violating the "monotonic per sender" rule in `gateway.md`.
- The documented outbound write priority (faults > replies > shutdown acks > heartbeats > events) is not implemented; the frame writer is a plain FIFO semaphore.
- Event-path allocation costs are avoidable: reflection-based `MXSTATUS_PROXY` field reads on every event, a defensive `Clone()` of every enqueued event, one pipe write + flush per event, and no envelope batching.
- Conventions are otherwise very good: MXAccess-aligned names, `sealed` classes, `Async` suffixes, file-scoped namespaces, nonce/credential redaction that actually works (`Bootstrap/WorkerLogRedactor.cs:16-25`); the main blemish is a split `_camelCase` vs `camelCase` private-field style between `Ipc/` and the rest of the worker.
- Test coverage is broad (STA scheduling, pump wake behavior, pipe session handshake/watchdog/shutdown races, frame protocol, conversion, event queue, alarm units) but misses STA thread death, wire sequence monotonicity, and the silent post-shutdown command drop.
## Findings
### Stability
**S1 — High.** A legitimately long `ReadBulk` self-faults as `StaHung` because no STA command can refresh the activity timestamp.
Evidence: `MxAccess/MxAccessSession.cs:919-931` waits up to `timeout` per uncached tag, sequentially per tag (`MxAccess/MxAccessSession.cs:876-888`); the pump step invoked during the wait calls `StaMessagePump.PumpPendingMessages()` directly and never calls `MarkActivity` (`Sta/StaRuntime.cs:90`, `MarkActivity` is private at `Sta/StaRuntime.cs:304-307`); the watchdog fires `StaHung` regardless of an in-flight command once staleness exceeds `HeartbeatStuckCeiling` (default 75 s) (`Ipc/WorkerPipeSession.cs:830-860`, `Ipc/WorkerPipeSessionOptions.cs:19`).
Failure scenario: `ReadBulk` with `timeout_ms = 5000` against 20 unreachable tags holds the STA ~100 s with `LastActivityUtc` frozen; at 75 s the worker emits `StaHung`, sets `_state = Faulted`, and from then on drops every completed command reply (`Ipc/WorkerPipeSession.cs:604-607`), so the gateway kills a healthy session. `docs/MxAccessWorkerInstanceDesign.md:688-690` assumes "no legitimate STA command should run that long without periodically refreshing activity", but no refresh mechanism exists.
Recommendation: have `StaRuntime.PumpPendingMessages()` (the `pumpStep` used by `ReadBulk`) call `MarkActivity()`, or clamp the total `ReadBulk` duration below the ceiling, or expose an activity-refresh hook to executors.
**S2 — Medium.** STA thread death after startup is silent and strands all future work.
Evidence: `Sta/StaRuntime.cs:255-259` catches any loop exception into `startupException` and sets `startedEvent` — but after startup `Start()` has already returned, so nothing ever observes the exception; it is not logged, not converted to a `WorkerFault`, and `shutdownRequested` stays false, so `InvokeAsync` (`Sta/StaRuntime.cs:165-178`) keeps enqueueing work items into a queue with no consumer and returns tasks that never complete. The only in-loop throw site is the pump wait failure (`Sta/StaMessagePump.cs:38-42`).
Failure scenario: `MsgWaitForMultipleObjectsEx` returns `WAIT_FAILED` once; the STA exits, the dispatcher's drain task wedges on the first stuck `InvokeAsync`, heartbeats keep flowing with a frozen `LastStaActivityUtc` and a non-empty `CurrentCommandCorrelationId`, and the worker is only declared hung after `HeartbeatStuckCeiling` — with the true root-cause exception permanently lost.
Recommendation: on `ThreadMain` exit, fail all queued and future `InvokeAsync` calls with the captured exception, and surface it (log + `WorkerFault`) instead of storing it in a write-only field.
**S3 — Medium.** Commands received after shutdown begins are silently dropped with no reply.
Evidence: `Ipc/WorkerPipeSession.cs:690-707``TryStartCommandTask` returns without any action when `_acceptingCommands` is false; no `WorkerUnavailable` reply, no `LogCommandResultDropped` diagnostic (that log fires only for completed-then-dropped replies, `Ipc/WorkerPipeSession.cs:604-607`).
Impact: a command racing `WorkerShutdown` leaves the gateway's correlation wait to expire on its own timeout with no trace. `docs/MxAccessWorkerInstanceDesign.md:697-699` says shutdown should "reject new commands" — the dispatcher layer does reply `WorkerUnavailable` (`Sta/StaCommandDispatcher.cs:117-123`), but this earlier gate replies with nothing.
Recommendation: write a `WorkerUnavailable` `WorkerCommandReply` (or at minimum log) for commands refused by the `_acceptingCommands` gate.
**S4 — Medium.** Envelope `sequence` can appear out of order on the wire.
Evidence: `NextSequence()` is called while building the envelope (`Ipc/WorkerPipeSession.cs:1005-1018`), but the writer lock is acquired later inside `WriteAsync` (`Ipc/WorkerFrameWriter.cs:68-77`). Command replies are written from independent per-command tasks (`Ipc/WorkerPipeSession.cs:690-706`) concurrently with the heartbeat and event-drain loops, so task B can take sequence n+1 yet win the write lock before task A's sequence n.
Impact: violates the `gateway.md` envelope rule "`sequence` is monotonic per sender" (gateway.md:313); any gateway-side consumer that trusts wire-order monotonicity mis-sorts frames. (Per-event ordering is safe — `WorkerSequence` on `MxEvent` is assigned inside the queue lock, `MxAccess/MxAccessEventQueue.cs:135-143` — but the envelope-level guarantee is broken.)
Recommendation: assign the envelope sequence inside the writer's critical section (e.g. a sequence-stamping callback under `_writeLock`).
**S5 — Medium.** One transient alarm poll failure kills the entire session, including its data subscriptions.
Evidence: `MxAccess/MxAccessStaSession.cs:278-291` — any exception from `handler.PollOnce()` records a fault on the shared event queue and permanently stops the poll loop; the drain loop turns that fault into session termination (`Ipc/WorkerPipeSession.cs:344-353` throws after writing the fault). The `FailoverAlarmConsumer` absorbs primary failures only in composite mode (`MxAccess/FailoverAlarmConsumer.cs:278-296`); in the default alarmmgr-only mode (`Ipc/WorkerPipeSession.cs:54` builds the handler with `standbyFactory: null`, and `AlarmCommandHandler.BuildConsumer` returns the bare consumer at `MxAccess/AlarmCommandHandler.cs:228-230`) a single `GetXmlCurrentAlarms2` COM error propagates unwrapped.
Failure scenario: one transient E_FAIL from the AVEVA alarm subsystem terminates a client session's `OnDataChange` stream even though the data path was healthy.
Recommendation: count consecutive alarm-poll failures (mirroring `FailoverSettings.Threshold`) before declaring the subscription dead, or scope the fault to the alarm feature rather than the whole session.
**S6 — Low.** Events still queued at graceful shutdown are discarded without a final drain, and the post-`Faulted` reply-drop policy discards legitimate late replies.
Evidence: `ShutdownAsync` writes the ack and returns `false` from dispatch (`Ipc/WorkerPipeSession.cs:657-688`, `396-398`); `RunMessageLoopAsync`'s `finally` then cancels the event-drain loop (`Ipc/WorkerPipeSession.cs:287-292`) with whatever remains in `MxAccessEventQueue` unshipped. Reply drops on `_state != Ready` are at `Ipc/WorkerPipeSession.cs:604-607`.
Impact: an `OnWriteComplete` raised during cleanup never reaches the gateway; acceptable for a closing session but undocumented — `docs/MxAccessWorkerInstanceDesign.md` does not state that shutdown discards the residual event queue.
Recommendation: drain the event queue once after `ShutdownGracefullyAsync` returns and before the ack, or document the discard.
**S7 — Low.** No `AppDomain.UnhandledException` hook; an exception on an unobserved thread crashes the process without a `WorkerFault` frame.
Evidence: `Program.cs:1-4` and `WorkerApplication.Run` (`WorkerApplication.cs:46-141`) install no unhandled-exception or unobserved-task handlers.
Impact: the gateway still detects the death via process exit and pipe closure, but the crash cause never reaches the fault channel or the worker log.
Recommendation: register `AppDomain.CurrentDomain.UnhandledException` (and `TaskScheduler.UnobservedTaskException`) to log through `IWorkerLogger` before exit.
**S8 — Low.** Top-level catch blocks log the exception type but never the message.
Evidence: `WorkerApplication.cs:112-139` logs only `exception_type` for protocol, pipe, and unexpected failures; similarly `HResultConverter.CreateSafeDiagnosticMessage` reduces every command exception to `Type: HRESULT 0x…` (`Conversion/HResultConverter.cs:46-49`).
Impact: root-causing a field failure from worker stderr requires reproducing it; the redactor (`Bootstrap/WorkerLogRedactor.cs`) already exists to make message logging safe.
Recommendation: log `exception.Message` (redacted) alongside the type at the process boundary; keep the credential-safe reply shape for IPC replies if that stripping is intentional parity policy.
Positive observations worth recording: partial pipe reads are handled correctly with a read-exactly loop and explicit EOF detection (`Ipc/WorkerFrameReader.cs:92-113`); gateway hello validation covers protocol version, session id, and nonce (`Ipc/WorkerPipeSession.cs:205-228`); handshake failures always attempt a structured fault before exiting (`Ipc/WorkerPipeSession.cs:192-203`); command ordering into the dispatcher is preserved because `ProcessCommandAsync` enqueues synchronously on the read-loop thread before its first await (`Ipc/WorkerPipeSession.cs:595`, `Sta/StaCommandDispatcher.cs:108-144`); the dispatcher's 128-entry bound provides real backpressure toward MXAccess (`Sta/StaCommandDispatcher.cs:11`, `125-131`); handle bookkeeping strictly follows the record-only-after-COM-success rules (`MxAccess/MxAccessSession.cs:187-307`, registry snapshots are copies so cleanup iteration is safe, `MxAccess/MxAccessHandleRegistry.cs:14-32`); and the value cache is evicted on `RemoveItem` to prevent stale reads across handle reuse (`MxAccess/MxAccessSession.cs:258-262`).
### Performance
**P1 — Medium.** `MXSTATUS_PROXY` conversion reflects on every field of every status of every event.
Evidence: `Conversion/MxStatusProxyConverter.cs:22-26` calls `ReadInt32Field` four times per status; each call performs `Type.GetField` + `FieldInfo.GetValue` + `Convert.ToInt32` (`Conversion/MxStatusProxyConverter.cs:83-103`). This runs inside the STA event handler path for every `OnDataChange`.
Impact: eight reflection operations plus boxing per event at data-change rates; the status type is always the interop `MXSTATUS_PROXY` struct, so the lookups are fully cacheable.
Recommendation: cache the four `FieldInfo` objects per `Type` (a two-entry static dictionary suffices), or cast to the interop struct directly.
**P2 — Low.** Every accepted event is defensively cloned on enqueue.
Evidence: `MxAccess/MxAccessEventQueue.cs:135``mxEvent.Clone()` for an event the mapper just built exclusively for this call (`MxAccess/MxAccessBaseEventSink.cs:210-256`); only the value-cache post-publish shares the original.
Impact: doubles protobuf allocation on the hottest path in the worker.
Recommendation: take ownership of the passed event in the queue and let the value cache store the copy (it already snapshots only value/quality/timestamp/statuses, `MxAccess/MxAccessValueCache.cs:44-57`).
**P3 — Low.** No event batching per envelope, one flush per event, and a 25 ms drain poll.
Evidence: `Ipc/WorkerPipeSession.cs:17-19` (25 ms interval, batch size 128 is only a read batch), `Ipc/WorkerPipeSession.cs:362-367` (one `WriteAsync` per event), `Ipc/WorkerFrameWriter.cs:71-72` (flush per frame). `gateway.md:849` lists "batch events from worker to gateway while preserving order" as the intended optimization; the `WorkerEnvelope` currently carries a single `WorkerEvent`.
Impact: at high data-change rates each event costs a semaphore round-trip, a pipe write, and a flush; idle-to-active latency is up to 25 ms.
Recommendation: acceptable for v1 parity; when throughput matters, add a repeated-event envelope body or coalesce flushes across a drained batch.
**P4 — Low.** The frame writer allocates a fresh `byte[]` per frame while the reader pools.
Evidence: `Ipc/WorkerFrameWriter.cs:63-66` (`new byte[frameLength]`) versus `Ipc/WorkerFrameReader.cs:55-77` (`ArrayPool<byte>.Shared`).
Recommendation: rent the write buffer from the same pool for symmetry.
### Conventions
**C1 — Low.** Private-field naming is split between two styles inside one project.
Evidence: `Ipc/` and `Bootstrap/` use `_camelCase` (`Ipc/WorkerPipeSession.cs:21-38`, `Ipc/WorkerFrameWriter.cs:13-15`, `Bootstrap/WorkerConsoleLogger.cs:10`), while `Sta/` and `MxAccess/` use bare `camelCase` (`Sta/StaRuntime.cs:10-24`, `MxAccess/MxAccessStaSession.cs:16-27`, `MxAccess/MxAccessSession.cs:11-16`). `docs/style-guides/CSharpStyleGuide.md:30-32` permits the underscore prefix "only when that pattern is already established in the project" — with both patterns established, the project has no single convention.
Recommendation: pick one style for the worker and migrate opportunistically; gateway-side code should be the tie-breaker.
**C2 — Low.** Documentation drift against `docs/WorkerSta.md` and `docs/MxAccessWorkerInstanceDesign.md`.
Evidence: the STA thread is named `"MxGateway.Worker.STA"` (`Sta/StaRuntime.cs:61`) but both docs state `ZB.MOM.WW.MxGateway.Worker.STA` (`docs/WorkerSta.md:23,30`, `docs/MxAccessWorkerInstanceDesign.md:254`); `docs/MxAccessWorkerInstanceDesign.md:653-654` says event queue depth and event sequence "are reported as zero until the event queue implementation owns those counters", but `CaptureHeartbeat` populates both from the live queue (`MxAccess/MxAccessStaSession.cs:375-380`).
Impact: CLAUDE.md requires docs to change with the source; operators grepping thread dumps for the documented name will miss the STA thread.
Recommendation: fix both docs (or rename the thread) in the next change touching this area.
**C3 — Low.** Boilerplate duplication in the IPC layer.
Evidence: seven pairs of trivially identical `CreateEnvelope`/`CreateBaseEnvelope` overloads (`Ipc/WorkerPipeSession.cs:920-1003`); eight constructor overloads on `WorkerPipeClient` (`Ipc/WorkerPipeClient.cs:36-140`).
Impact: purely maintenance noise; every new envelope body means two more copy-paste methods.
Recommendation: collapse to a single `CreateEnvelope(Action<WorkerEnvelope> setBody)` or a switch on the body message.
Positive observations: MXAccess-aligned naming is consistently applied (`MxStatusProxy`, `ServerHandle`, `ItemHandle`, `HResult`, event family names match the contract); every class is `sealed`; `Async` suffixes are correct throughout; the net48 constraints are handled cleanly (plain `readonly struct` instead of records, documented at `MxAccess/MxAccessValueCache.cs:165-168`); and empirical COM findings are captured in code comments with dates and doc references (`MxAccess/WnWrapAlarmConsumer.cs:113-119`, `264-274`), which is exactly the "explain why" documentation style the repo mandates.
### Underdeveloped
**U1 — Medium.** The documented outbound write priority is not implemented.
Evidence: `docs/MxAccessWorkerInstanceDesign.md:606-613` specifies write priority faults > command replies > shutdown acks > heartbeats > events; the worker has no prioritized queue — all writers contend on a single FIFO `SemaphoreSlim` (`Ipc/WorkerFrameWriter.cs:14`, `68-77`), and events are written inline by the drain loop (`Ipc/WorkerPipeSession.cs:362-367`).
Impact: with a deep event backlog draining, a `WorkerFault` or command reply queues behind up to 128 event writes; on a slow pipe this delays the gateway's fault reaction.
Recommendation: either implement a small priority write scheduler or amend the design doc to state that FIFO ordering was accepted for v1.
**U2 — Low.** Gateway death exits with the wrong exit code.
Evidence: pipe EOF surfaces as `WorkerFrameProtocolException(EndOfStream)` (`Ipc/WorkerFrameReader.cs:104-109`), which `WorkerApplication` catches first and maps to `WorkerExitCode.ProtocolViolation` (6) (`WorkerApplication.cs:110-119`) even though a distinct `PipeConnectionFailed` (5) exists (`Bootstrap/WorkerExitCode.cs:10`) and the in-session fault mapping does distinguish `PipeDisconnected` (`Ipc/WorkerPipeSession.cs:1212-1220`).
Impact: post-mortem triage of orphaned workers ("did the gateway die or did the worker misbehave?") reads the wrong signal from the exit code.
Recommendation: special-case `WorkerFrameProtocolErrorCode.EndOfStream` to exit `PipeConnectionFailed`.
**U3 — Low.** Event-queue overflow behavior diverges from the documented sequence, and exits as `UnexpectedFailure`.
Evidence: `docs/MxAccessWorkerInstanceDesign.md:615-624` says overflow should "stop accepting new commands" and "let the gateway close or kill the worker"; the implementation instead terminates the session immediately — the drain loop writes the fault then throws (`Ipc/WorkerPipeSession.cs:344-353`), unwinding `RunAsync` into the generic handler and exit code 1 (`WorkerApplication.cs:131-139`).
Impact: fail-fast is arguably stronger than documented (acceptable), but the designed fault path is indistinguishable from a crash by exit code.
Recommendation: catch the drain-fault termination in `WorkerApplication` and exit with a dedicated code; update the doc to match the implemented policy.
**U4 — Low.** Command start/end logging with correlation id is absent.
Evidence: `docs/MxAccessWorkerInstanceDesign.md:790-791` lists "command start/end with correlation id" among required worker logs; the only per-command log is the dropped-reply diagnostic (`Ipc/WorkerPipeSession.cs:645-655`). `StaCommand.EnqueueTimestamp` is captured (`Sta/StaCommand.cs:36`) but never used for latency measurement.
Recommendation: add optional (level-gated) start/end logging in `StaCommandDispatcher.ExecuteQueuedCommandAsync`, which already brackets each command (`Sta/StaCommandDispatcher.cs:265-281`).
**U5 — Low.** Test-coverage gaps around the failure modes found above.
Evidence: `Worker.Tests` covers the pump wake behavior (`Sta/StaMessagePumpTests.cs`), dispatcher ordering/cancellation/shutdown (`Sta/StaCommandDispatcherTests.cs`), the pipe-session handshake, heartbeat, watchdog (including the stuck ceiling), control commands, shutdown races and late-reply drops (`Ipc/WorkerPipeSessionTests.cs:25-1030`), frame protocol, conversion, the event queue, and the alarm units. Not covered: STA thread death mid-run (S2), wire-level envelope sequence monotonicity under concurrent writers (S4), the silent no-reply drop at the `_acceptingCommands` gate (S3), and the `ReadBulk`-exceeds-ceiling false fault (S1).
Recommendation: add tests alongside the fixes; the existing fake-runtime harness in `WorkerPipeSessionTests` supports all four.
Command-surface parity is otherwise complete: all 18 core MXAccess methods, the 11 bulk variants, `ReadBulk` with cached and snapshot paths, the five diagnostics commands, and the five alarm commands are dispatched (`MxAccess/MxAccessCommandExecutor.cs:95-132`), matching the `gateway.md` command list; `OperationComplete` is emitted only from the native handler and nothing synthesizes events (`MxAccess/MxAccessBaseEventSink.cs:160-171`); buffered payloads preserve raw data-type metadata when conversion is incomplete (`MxAccess/MxAccessEventMapper.cs:237-264`, `379-423`).
## Top 5 recommendations
1. **Fix the ReadBulk/watchdog false positive (S1).** Make the `pumpStep` used by long-running commands refresh `LastActivityUtc` (a one-line `MarkActivity()` in `StaRuntime.PumpPendingMessages`), or clamp total `ReadBulk` duration below `HeartbeatStuckCeiling`. This is the only path found where a healthy worker declares itself hung and then silently drops all replies.
2. **Make STA thread death observable (S2).** On `ThreadMain` exit, complete queued and future `InvokeAsync` calls with the captured exception and emit a `WorkerFault`; never leave the exception in a write-only field.
3. **Reply to commands refused during shutdown (S3).** The `_acceptingCommands` gate should produce a `WorkerUnavailable` reply, matching the dispatcher-level rejection the design docs describe.
4. **Assign envelope sequence under the write lock (S4)** so the wire honors the "monotonic per sender" envelope rule; add a concurrent-writer test.
5. **Cut event hot-path cost (P1, P2).** Cache `MXSTATUS_PROXY` `FieldInfo` lookups and remove the per-event `Clone()` in `MxAccessEventQueue.Enqueue`; both are localized changes with no protocol impact, and they precede any need for envelope batching (P3).