Joseph Doherty
09ccd9561f
Code-review follow-up on fix/gwc-26-27-alarm-attach. ApplyReconcile's snapshot-derived feed repairs are at-least-once, not exactly-once: a reconcile reads the worker's current state while the matching live transition may still be buffered in the monitor's lease, so both broadcast and the duplicates are indistinguishable on the alarm feed. This pre-dates the acked-state delta — the Raise/Clear presence repair has always had it, since nothing serializes a reconcile pass against the in-flight live stream — so closing it (serialization or timestamp dedup) stays out of scope for a P2 fix. Documented instead, with the consumer contract stated explicitly (apply transitions idempotently, never as an increment or toggle): - ApplyReconcile gains a "Delivery semantics" comment. - gateway.md softens the "defense in depth" prose to state the semantics. - docs/Sessions.md carries the same caveat on the alarm-feed description. - Tracker change-log records it as a known pre-existing characteristic and a candidate finding for the next review cycle. Also hoists the ChannelWorkerClient fake — duplicated across the three alarm test files — into TestSupport/, dropping the usings it took with it.
20 KiB
20 KiB
MxAccessGateway — Remediation Tracking (2026-07-12 review)
Master progress tracker for the 2026-07-12 follow-up architecture review. Generated 2026-07-13.
Source review: ../00-overall.md · Per-domain remediation designs are linked below and hold the full Finding / Impact / Design / Implementation / Verification for every entry here. The first-cycle tracker (../../remediation/00-tracking.md) remains the record for the original 153 findings; this document tracks only the 47 new IDs (GWC-24+, WRK-21+, IPC-23+, SEC-31+, CLI-35+, TST-25+) plus the old-tracker actions listed at the end.
How to use this document
- Each finding has a stable ID that never changes. Cite it in commits, branches, and PRs (e.g.
fix(GWC-25): empty-ring ReplayGap sentinel). - The Status column is the single source of truth for progress. Update it in the same change that lands the fix.
- Do the work in roadmap-tier order (P0 → P1 → P2), respecting the
Depcolumn and the cross-cutting clusters below — several fixes are one change set across two domains and must land together. - When a fix lands: flip Status to
Doneand, per the repo rule, update the affected docs in the same commit.
Status legend: Not started · In progress · In review · Done · Won't fix (record why in the domain doc) · N/A (informational / decision recorded, no action).
Severity roll-up
| Domain | Doc | High | Medium | Low | Info | Total |
|---|---|---|---|---|---|---|
| Gateway core | 10-gateway-core.md | — | 2 | 4 | 1 | 7 |
| Worker | 20-worker.md | — | 1 | 7 | — | 8 |
| Contracts & IPC | 30-contracts-ipc.md | — | 3 | 5 | 2 | 10 |
| Security & dashboard | 40-security-dashboard.md | — | 1 | 4 | 1 | 6 |
| Clients | 50-clients.md | — | 5 | 6 | — | 11 |
| Testing, docs & gaps | 60-testing-docs-gaps.md | 1 | 2 | 3 | — | 6 |
| Total | 1 | 14 | 29 | 4 | 48 |
Roadmap-tier roll-up
| Tier | Meaning | Count | Findings |
|---|---|---|---|
| P0 | Correctness & safety — all small-to-medium | 10 | GWC-25, WRK-21, IPC-23, IPC-24, IPC-25, IPC-30, SEC-31, SEC-32, CLI-35, CLI-36 |
| P1 | Process & hardening | 12 | GWC-24, WRK-26, SEC-33, SEC-36, CLI-37, CLI-38, CLI-39, CLI-42, CLI-45, TST-25, TST-26, TST-27 |
| P2 | Completeness & polish | 10 | GWC-26, GWC-27, GWC-28, WRK-25, IPC-26, IPC-27, SEC-34, TST-28, TST-29, TST-30 |
| — | Not individually in the roadmap (Lows/Infos) | 16 | GWC-29/30, WRK-22/23/24/27/28, IPC-28/29/31/32, SEC-35, CLI-40/41/43/44 |
P0 — do first (10)
Sequenced by cluster; a cluster is one change set.
| ID | Sev | Eff | Dep | Status | Title |
|---|---|---|---|---|---|
| GWC-25 | Medium | S | CLI-35/36 (coord) | Not started | Empty-ring ReplayGap sentinel carries oldest_available_sequence = 0, dead-streaming a compliant client |
| CLI-35 | Medium | S | GWC-25 (coord) | Not started | Python CLI stream-events crashes on a ReplayGap |
| CLI-36 | Medium | S | GWC-25 (coord) | Not started | Go CLI stream-events silently destroys the ReplayGap signal |
| WRK-21 | Medium | M | owns IPC-23 fix; WRK-28 same batch | Not started | DrainEvents bound is count-based only; oversized reply kills the session and loses the drained events |
| IPC-23 | Medium | S | WRK-21 | Not started | DrainEvents contract requirements (reply fits negotiated max, no event loss, drain-until-empty) + proto-comment/doc wave |
| IPC-30 | Low | M | WRK-21 (same batch) | Not started | Oversized event frame stays session-fatal by design, but the death becomes structured (fault frame + logged identity) |
| SEC-31 | Medium | M | — | Not started | Failure limiter partitions on attacker-controlled key id and blocks before verification (lockout DoS) |
| SEC-32 | Low | S | SEC-31 | Not started | Failure-limiter LRU flushable by junk-token spray; token prefix never validated |
| IPC-24 | Medium | S | — | Not started | CI's unconditional Java churn-revert masks real drift |
| IPC-25 | Medium | M | — | Not started | Regenerate stale Go/Python worker bindings + add binding-freshness guard (Check 4) to check-codegen.ps1 |
Finding registers by domain
Full design + implementation for each row lives in the linked domain doc under its ID heading.
Gateway core — 10-gateway-core.md
| ID | Sev | Tier | Eff | Dep | Status | Title |
|---|---|---|---|---|---|---|
| GWC-24 | Medium | P1 | M | GWC-21 (coord, old tracker) | Not started | Unbounded event staging channel: sustained slow drain grows memory silently and invisibly |
| GWC-25 | Medium | P0 | S | CLI-35/36 (coord) | Not started | Empty-ring ReplayGap sentinel carries oldest_available_sequence = 0 |
| GWC-26 | Low | P2 | M | GWC-27 | Done | Alarm monitor attaches its subscriber after SubscribeAlarms; window transitions bypass the feed |
| GWC-27 | Low | P2 | S | GWC-26 | Done | AttachInternalEventSubscriber bypasses the readiness gate; premature attach poisons the distributor |
| GWC-28 | Low | P2 | S | GWC-10 (coord, old tracker) | Not started | Gateway→worker envelope sequence stamped at creation, not at write |
| GWC-29 | Low | — | S | — | Not started | Invoke deep-clones the entire request only to discard the cloned command |
| GWC-30 | Info | — | S | — | Not started | Frame reader allocates a fresh 4-byte length-prefix array per frame |
Worker — 20-worker.md
| ID | Sev | Tier | Eff | Dep | Status | Title |
|---|---|---|---|---|---|---|
| WRK-21 | Medium | P0 | M | IPC-23 (fix owned here); WRK-28 | Not started | DrainEvents bound count-based only; oversized reply kills session and loses drained events |
| WRK-22 | Low | — | S | IPC-26 (fix owned here) | Not started | Cancelled WriteAsync leaves its frame queued; it is still written later |
| WRK-23 | Low | — | S | WRK-21 | Not started | Rejected frames consume sequence numbers, producing wire gaps |
| WRK-24 | Low | — | S | — | Not started | AdoptNegotiatedMaxMessageBytes has no lower-bound sanity check |
| WRK-25 | Low | P2 | S | WRK-22 (shared seam) | Not started | WRK-12 flush coalescing never engages on the event hot path |
| WRK-26 | Low | P1 | S | WRK-23 (soft); discharges IPC-29 | Not started | Write-priority and overflow doc drift from the WRK-07 change |
| WRK-27 | Low | — | S | — | Not started | Alarm poll bypasses the watchdog's in-flight suppression (15 s vs 75 s) |
| WRK-28 | Low | — | S | WRK-21 (same batch) | Not started | 10,000 drain cap is a duplicated magic constant |
Contracts & IPC — 30-contracts-ipc.md
| ID | Sev | Tier | Eff | Dep | Status | Title |
|---|---|---|---|---|---|---|
| IPC-23 | Medium | P0 | S | WRK-21 (mechanics) | Not started | DrainEvents byte-blindness — contract requirements + proto-comment/doc wave |
| IPC-24 | Medium | P0 | S | — | Not started | CI's unconditional Java churn-revert masks real drift |
| IPC-25 | Medium | P0 | M | — | Not started | Stale Go/Python worker bindings: regenerate (pinned toolchains) + check-codegen Check 4 |
| IPC-26 | Low | P2 | S | WRK-22 (mechanics) | Not started | Cancelled write leaves ghost frame — cancelled means never written |
| IPC-27 | Low | P2 | S | — | Not started | Descriptor-freshness test blind to enums/services/galaxy descriptor |
| IPC-28 | Low | — | S | — | Not started | docs/Grpc.md missing CommandTooLarge → ResourceExhausted mapping |
| IPC-29 | Low | — | S | WRK-26 (discharged by) | Not started | WorkerFrameProtocol.md missing write-scheduling/sequencing section |
| IPC-30 | Low | P0 | M | WRK-21 (same batch) | Not started | 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-32 | Info | — | S | IPC-25 (folded in) | Not started | check-codegen banner relabel 1/4…4/4 |
Security & dashboard — 40-security-dashboard.md
| ID | Sev | Tier | Eff | Dep | Status | Title |
|---|---|---|---|---|---|---|
| SEC-31 | Medium | P0 | M | — | Not started | Failure limiter: composite (peer, key-id) partitions + cross-peer aggregate with probe admission |
| SEC-32 | Low | P0 | S | SEC-31 | Not started | Limiter LRU flushable by junk-token spray; validate token shape, cap per-peer partitions |
| SEC-33 | Low | P1 | M | old SEC-23 (co-locate) | Not started | Host-meaningful path rooting; drop Windows literals from appsettings; validate Galaxy SnapshotCachePath |
| SEC-34 | Low | P2 | S | — | Not started | Verification cache: expiry outlives TTL; Invalidate races in-flight repopulation |
| SEC-35 | Info | — | S | — | N/A | Production hard-stops key on exact Production environment name (doc-only) |
| SEC-36 | Low | P1 | M | cross-repo scadaproj/infra/glauth |
Not started | Committed dev LDAP service-account password: rotate, remove, move dev channel to user-secrets |
Clients — 50-clients.md
| ID | Sev | Tier | Eff | Dep | Status | Title |
|---|---|---|---|---|---|---|
| CLI-35 | Medium | P0 | S | GWC-25 (coord) | Not started | Python CLI stream-events crashes on a ReplayGap |
| CLI-36 | Medium | P0 | S | GWC-25 (coord) | Not started | Go CLI stream-events silently destroys the ReplayGap signal |
| CLI-37 | Medium | P1 | M | CLI-38 (co-land) | Not started | Status-array validation must branch on category per the proto contract (4-vs-1 divergence) |
| CLI-38 | Medium | P1 | S | — | Not started | Align .NET/Go/Java on hresult < 0 — lands old CLI-08, cures design-doc drift |
| CLI-39 | Medium | P1 | S | CLI-35..38, CLI-45 (land last) | Not started | Bump client versions off published 0.1.2 (converge on 0.2.0); registry-collision guard in pack-clients.ps1 |
| CLI-40 | Low | — | M | — | Not started | Port the exact-secret credential scrub to Rust/Java/.NET |
| CLI-41 | Low | — | M | — | Not started | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem |
| CLI-42 | Low | P1 | S | — | Not started | Document the vendored Rust proto layout (CLI-02's missing doc half) |
| CLI-43 | Low | — | S | — | Not started | Java style guide still prescribes "Java 21 preferred" |
| CLI-44 | Low | — | S | — | Not started | Go event goroutine can mislabel a genuine terminal error as ErrSlowConsumer |
| CLI-45 | Low | P1 | M | — | Not started | Standardize CLI credential env-var names; fail fast on missing/empty passwords |
Testing, docs & gaps — 60-testing-docs-gaps.md
| ID | Sev | Tier | Eff | Dep | Status | Title |
|---|---|---|---|---|---|---|
| TST-25 | High | P1 | M | unlocks old TST-05, TST-24 | Done | Windows/x86 test tier has zero automation — SSH-driven windev CI job |
| TST-26 | Medium | P1 | S | TST-25 (same commit) | Done | Docs/scripts describe removed CI jobs; Generated/-guard reattributed to check-codegen |
| TST-27 | Medium | P1 | S | — | Not started | ShowTagValues config row still says "Reserved" after SEC-25 made the flag live |
| TST-28 | Low | P2 | S | relates IPC-02 (old) | Not started | Gateway-side max_frame_bytes handshake untested in the CI-run suite |
| TST-29 | Low | P2 | S | — | Not started | Retire oldtasks.md (fold Phase-5 governance into DesignDecisions.md); delete root artifacts |
| TST-30 | Low | P2 | M | — | Not started | Single shared Gitea runner is a CI throughput/availability bottleneck (cross-repo contention, no run cancel/delete API) |
Cross-cutting clusters
Sequence these together rather than piecemeal — several are one change set spanning two domains:
- Drain / oversized-frame cluster (P0): WRK-21 + IPC-23 + IPC-30, with WRK-28 and WRK-23 in the same batch. One worker change set in
WorkerPipeSession.cs/WorkerFrameWriter.cs; WRK-21 owns the byte-budgeted drain and must satisfy IPC-23's requirements R1–R3; IPC-30's structured-fault seam lands in the same drain loop. IPC-23's proto-comment edits trigger the full regen fan-out (Generated/, Rust vendored, Go/Java, descriptor set) — land with WRK-21 so the wave happens once. One windev x86 verification run for the cluster. - ReplayGap end-to-end (P0): GWC-25 (server sentinel arithmetic) + CLI-35 (Python CLI) + CLI-36 (Go CLI). Independently landable; the e2e resume walk validates only when all three are in.
- Auth limiter (P0): SEC-31 + SEC-32 — same component, one change set, one test suite.
- Codegen freshness (P0): IPC-24 + IPC-25 (+ IPC-32 folded in). Both edit
.gitea/workflows/ci.yml— coordinate the branch with TST-25, which touches the same file. - Windows-tier automation (P1): TST-25 + TST-26 (same commit). Unlocks old TST-05/TST-24 and provides CI evidence for every windev-verified cluster above; until it lands, record windev runs in this tracker's change log.
- Client conformance + release train (P1): CLI-37 + CLI-38 co-land (one conformance commit; closes old CLI-08), then CLI-45, with CLI-40/41 fixtures as follow-ups; CLI-39 lands last so published 0.2.0 carries the conformant behavior. Shared fixtures under
clients/proto/fixtures/behavior/; update CrossLanguageSmokeMatrix.md/ClientLibrariesDesign.md same-commit. Do not republish regenerated bindings (IPC-25) before CLI-39 resolves. - Doc-drift batch (P1): TST-27 + WRK-26 (discharges IPC-29) + CLI-42 + IPC-28 + SEC-35's doc note — one sweep commit is fine.
- Backpressure follow-on (P1): GWC-24, coordinating with still-open old GWC-21 (
EventChannelFullModeTimeoutconfigurability).
Old-tracker actions (../../remediation/00-tracking.md)
- Close CLI-24 and CLI-34 as
Done(incidentally fixed; evidence in ../50-clients.md). - When CLI-38 lands, close old CLI-08 with a pointer here.
- When WRK-26 lands, its doc section also discharges the WorkerFrameProtocol gap; when TST-25 lands, revisit old TST-05 (scheduled live smoke) and TST-24 (client wire tests), which it unlocks.
Change log
| Date | Change |
|---|---|
| 2026-07-13 | Initial tracking doc generated from the six domain remediation designs. All 47 findings Not started (IPC-31, SEC-35 N/A). |
| 2026-07-13 | TST-25/TST-26 → In progress (branch fix/tst-25-windev-ci). Added scripts/ci/{windev-worker-ci.ps1,run-windev-ci.sh,windev.known_hosts}, windows-x86 (per-push) + nightly-windev (scheduled) jobs in ci.yml, and the TST-26 doc/comment fixes (GatewayTesting.md, Contracts.md, check-codegen.ps1). Mechanism hand-verified on windev: build→0, bogus-SHA→nonzero (lock released), test→356 passed/0 failed in ~50s (per-push stays test, no demotion), and run-windev-ci.sh SSH+EncodedCommand exit-code propagation confirmed. |
| 2026-07-13 | Operator bring-up complete: dedicated CI ed25519 key installed in windev administrators_authorized_keys (authorized into dohertj2, which owns the working MXAccess/toolchain env — a fresh OS account would break the build; the key is independently revocable), Gitea secrets WINDEV_SSH_KEY/WINDEV_SSH_KNOWN_HOSTS + variable WINDEV_SSH_USER=dohertj2 stored, runner→10.100.0.48:22 egress verified on the traefik net, issue-write confirmed. TST-25/TST-26 → Done: credentialed windows-x86 ran GREEN on d769244 (Gitea run #37) — Linux runner SSHed windev, checked out the SHA in C:\build\mxaccessgw-ci under lock, ran the x86 Worker build + Worker.Tests, exit 0; nightly-windev correctly skipped on the push event. Branch merged to main. Follow-ups (old tracker): revisit TST-05 (scheduled live smoke — now covered by nightly-windev) and TST-24 (client wire tests) which this unlocks. |
| 2026-07-13 | Ran the TST-25 acceptance checks (scripts/ci/README.md) — they caught two real CI defects, both fixed on fix/tst-25-ci-key-log-leak: (1) CI SSH key leaked in cleartext in the windows-x86 step env echo (Gitea's line-oriented masker missed the multiline PEM) — rotated the CI key on windev (old pubkey revoked), stored the key base64-encoded so the masker redacts it to *** (confirmed on run #38), taught run-windev-ci.sh to decode, dropped the redundant public known-hosts secret from the job env; (2) bootstrap lock race — run-windev-ci.sh's pre-hand-off git fetch/checkout ran outside the worktree lock, so concurrent runs collided on .git/index.lock; the bootstrap now holds the lock (ps1 re-uses it via MXGW_CI_LOCK_HELD), retest confirmed clean serialization. Also deflaked SessionManagerTests fail-fast timing assertions (absolute <100ms wall-clock bound flaked under CI load; now anchored to the configured timeout / dropped for the zero-timeout case). Checks passed: unreachable-host fast-fail (exit 255/15s), deliberate-red propagation (Worker.Tests failure → exit 1), lock concurrency (2nd run waits), no-key-in-logs (masked). Merge target df7e20d verified GREEN via the local windev path (Worker build + 356 tests); merged to main 19cbf7b. Check 6 (forced-failure nightly issue): issue endpoint+token proven live at bring-up (#124); in-CI forced-failure probe abandoned to shared-runner congestion (residual if: failure() gating is standard Actions). |
| 2026-07-13 | New finding TST-30 (Low/P2) added — surfaced during TST-25 acceptance: CI runs on a single shared gitea-runner (maxParallel=1, co-located 10.100.0.35) interleaved with dohertj2/lmxopcua, and Gitea 1.26 exposes no run cancel/delete, so queue latency is unbounded under cross-repo contention and the runner is a single point of failure. Design: add a second/labelled runner + document the no-cancel reality and the run-windev-ci.sh bypass. Roll-ups updated (Testing Low 2→3, total 47→48; P2 9→10). |
| 2026-08-07 | GWC-27 → Done, GWC-26 → Done (branch fix/gwc-26-27-alarm-attach). GWC-27: GatewaySession.AttachInternalEventSubscriber now mirrors AttachEventSubscriber's readiness gate under _syncRoot, before EnsureDistributorCreated, so a premature attach can no longer latch a poisoned distributor. GWC-26: the alarm monitor takes its internal lease directly from the session before SubscribeAlarms and drains it after the first reconcile; ISessionManager.ReadAlarmEventsAsync removed (zero remaining callers); ApplyReconcile now broadcasts an Acknowledge feed transition for a both-present alarm whose state advanced to ActiveAcked (feed-level repair on AlarmFeedMessage, not MxEvent synthesis). New tests GatewaySessionTests.AttachInternalEventSubscriberBeforeReadyThrowsAndDoesNotPoisonDistributor and GatewayAlarmMonitorAttachOrderTests (TransitionsDuringSubscribeWindow_StillReachTheAlarmFeed, ApplyReconcileBroadcastsAcknowledgeDelta); the alarm-monitor fakes now hand the monitor a real Ready GatewaySession with a dashboard mirror so the window is actually reproducible. Verification: NonWindows build 0 warnings/0 errors; GatewayAlarmMonitor 16 passed, SessionManagerTests 38 passed, GatewaySessionTests 19 passed, AlarmFailoverEndToEndTests 2 passed. |
| 2026-08-07 | Code review of fix/gwc-26-27-alarm-attach surfaced a known pre-existing characteristic, now documented: the alarm monitor's reconcile-derived feed repairs are at-least-once, not exactly-once. A reconcile reads the worker's current state while the matching live transition may still be buffered in the monitor's internal lease, so both broadcast and the duplicates are indistinguishable on the alarm feed (StreamAlarms + dashboard alarm hub). This pre-dates GWC-26 — the Raise/Clear presence repair has always had it, since nothing serializes a reconcile pass against the in-flight live stream — so closing it (reconcile/live serialization or transition-timestamp dedup) was ruled out of scope for a P2 fix. Documented instead in GatewayAlarmMonitor.ApplyReconcile, gateway.md, and docs/Sessions.md, with the consumer-side contract stated explicitly (apply transitions idempotently — "set this alarm to this state", never increment/toggle). Candidate finding for the next review cycle. |