diff --git a/archreview/plans/R3-04-one-role-authority-plan.md b/archreview/plans/R3-04-one-role-authority-plan.md index 0b33665d..c70d614c 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md +++ b/archreview/plans/R3-04-one-role-authority-plan.md @@ -552,3 +552,106 @@ cycle would have been (both derivations are proven against literally identical i nodes' role sources interleaves site-a-2 and site-b-2 on adjacent lines, so a clean git-level split into the plan's two separate commits was not practical; both tasks' edits landed in one commit (`985de0b6`). The rig was NOT brought up as part of either task, per the brief. + +## Review outcome + +A code review after T1–T12 returned verdict **FIX-FIRST**, two items. Both are fixed, each in its +own TDD commit, on top of the T1–T12 work above. + +### 1. [Blocking] T7's post-overlay guard was structurally unreachable + +**The finding.** `AkkaClusterOptionsValidator.ValidateAgainstEffectiveRoles` compared +`options.Roles` against `_effectiveRoles.Gossiped` — but `ServiceCollectionExtensions.cs`'s +PostConfigure (T2's OWN structural-identity fix) unconditionally sets +`options.Roles = [.. effectiveRoles.Gossiped]` first. The two values are therefore **equal by +construction**, on every real host, always — the guard could never observe a divergence no matter +what a role overlay did to `Cluster:Roles`. My original two tests (now deleted) hand-built an +`AkkaClusterOptions` instance directly and set `.Roles` to a "mismatched" value bypassing +PostConfigure entirely — which exercised the comparison logic in isolation but never proved the +guard reachable through the real pipeline. **Concretely, before this fix: an overlay that set +`Cluster:Roles` was silently ignored (PostConfigure overwrote it back to the pre-overlay value), not +failed at boot** — the exact opposite of the guard's purpose (choice 4: "a role overlay may not set +Cluster:Roles"). + +**The fix.** `AkkaClusterOptionsValidator` now also takes an optional `IConfiguration?` (default +null, same pattern as `EffectiveRoles?` — so every pre-existing zero/one-arg test construction keeps +compiling and behaving as before). The guard re-reads the RAW `Cluster:Roles` section directly off +the LIVE configuration (`_configuration.GetSection(EffectiveRoles.ConfiguredRolesKey).Get()`) +rather than the bound options, normalizes it by reusing `EffectiveRoles.Resolve(raw, envRoles: null)` +(one rule, stated once — archreview theme 4, rather than re-implementing the configured-path +normalization a second time), and compares that against `_effectiveRoles.Gossiped`. Skipped entirely +when the raw post-overlay value is empty — required for correctness, not merely tidiness: an +`OTOPCUA_ROLES`-only node (most of production, per CLAUDE.md) has an empty `Cluster:Roles` both +before AND after any overlay, and comparing "empty" against a non-empty env-derived `Gossiped` list +would be a permanent false positive on every one of those nodes. `AddOtOpcUaCluster` now also +`TryAddSingleton(configuration)` so a bare `ServiceCollection` test harness (not just +`WebApplicationBuilder`, which auto-registers `IConfiguration`) can supply it. + +**The tests.** Both guard tests are rewritten to go through the REAL `AddOtOpcUaCluster` + +PostConfigure pipeline, over a `ConfigurationManager` (not a one-shot `ConfigurationBuilder`) — the +same "add a provider to the SAME live object after the fact" shape `WebApplicationBuilder.Configuration` +has, which is what lets a role overlay be simulated faithfully: `EffectiveRoles.Resolve(configuration)` +is called and its result registered BEFORE a second `AddInMemoryCollection` call mutates the same +`configuration` object with different `Cluster:Roles` values, mirroring `Program.cs`'s own +resolve-then-`AddJsonFile(overlay)` sequencing exactly. Confirmed red first: the "changes" test +failed with "should throw OptionsValidationException but did not" against the pre-fix (options.Roles +-comparing) validator — direct proof of the reviewer's structural-unreachability claim. Two +supporting tests were added beyond the two named in the review comment: `No_overlay_at_all_passes` +(the ordinary case) and `EffectiveRoles_injected_without_IConfiguration_skips_the_guard_entirely` +(the two-optional-dependency ctor's other skip leg). One self-inflicted false-positive surfaced and +was fixed along the way: the new catch-block's error message split `"Cluster:Roles."` onto its own +string-literal token, which `RoleAuthorityGuardTests`' own space-inside-the-literal heuristic (T8) +correctly flagged as NOT looking like an operator sentence — merging it back into one literal +containing surrounding words fixed it without weakening the guard. + +### 2. [Important] `ClusterRoleInfo._roleLeaders` must stay `Ordinal`, not `OrdinalIgnoreCase` + +**The finding.** T5 moved BOTH `_roleLeaders` and `_membersByRole` to `OrdinalIgnoreCase` under one +shared rationale (defence in depth against a miscased rolling-upgrade peer). That rationale is right +for `_membersByRole` ("every member carrying a spelling of this role" — merging casings is the +point) but wrong for `_roleLeaders`: Akka computes — and raises `RoleLeaderChanged` for — role +leadership **per exact gossiped role string**. "driver" and "Driver" are two entirely distinct roles +to Akka's own role-leader machinery, each independently electing its own leader. Keying +`_roleLeaders` case-insensitively collapses both onto ONE dictionary slot, so whichever +`RoleLeaderChanged` event landed last silently overwrites the other's leader — turning two real, +independent facts Akka tracks separately into one, wrong, merged fact. + +**The fix.** `_roleLeaders` reverted to `StringComparer.Ordinal`; `_membersByRole` stays +`OrdinalIgnoreCase` (the split itself, not merely one value, is now documented in the field +comment). No other production call site touches `_roleLeaders`'s comparer, so this is a one-line +revert plus doc. + +**The test.** `ClusterRoleInfoRoleLeaderCaseTests.RoleLeaderChanged_for_driver_then_Driver_keeps_distinct_entries` +— a real two-node cluster (the only faithful option: `HandleRoleLeaderChanged` is `internal` with no +`InternalsVisibleTo`, so it cannot be unit-tested directly, and `ClusterRoleInfo`'s constructor needs +a genuinely clustered `ActorSystem`). Node A gossips only `"driver"`, node B (joining the same +cluster) gossips only `"Driver"` — each is therefore the sole, and so leader, member of its own +exact-string role. Confirmed red first: against the pre-fix (`OrdinalIgnoreCase`) comparer, the test +failed with `driverLeader` and `capitalDriverLeader` resolving to the SAME address — the exact +collision the review predicted, reproduced live rather than merely reasoned about. + +### Optional item — done + +Added one sentence to `docs/ServiceHosting.md`'s "Per-role configuration overlays" section: a plain +role's overlay-filename fragment is always lower-case regardless of how it was spelled in +`Cluster:Roles`, but a `cluster-{ClusterId}` role's suffix carries into the filename verbatim (since +`EffectiveRoles.Gossiped` preserves that spelling by design) — e.g. +`Cluster:Roles: ["driver", "cluster-SITE-A"]` loads `appsettings.cluster-SITE-A-driver.json`. + +### Verification + +`dotnet build ZB.MOM.WW.OtOpcUa.slnx` → 0 errors throughout. + +- After item 2 (`ClusterRoleInfo`): `Cluster.Tests` 241 → **242/242** (+1, + `ClusterRoleInfoRoleLeaderCaseTests`). +- After item 1 (T7 guard): `Cluster.Tests` 242 → **244/244** (net +2: the two old hand-built T7 + tests deleted, four new ones added — + `An_overlay_that_changes_Cluster_Roles_after_resolution_fails_naming_both_views`, + `An_overlay_that_repeats_the_same_Cluster_Roles_passes`, `No_overlay_at_all_passes`, + `EffectiveRoles_injected_without_IConfiguration_skips_the_guard_entirely` — the pre-existing + `No_EffectiveRoles_injected_skips_the_guard_entirely` was kept unchanged). +- `Host.Tests` **106/106**, unchanged by either fix (`Program.cs` itself was not touched). +- `ControlPlane.Tests` **267/267** — an extra safety check beyond what was asked, since + `ClusterRoleInfo` is a shared dependency of `RedundancyStateActor` et al. + +No pushes, no merges. diff --git a/docs/ServiceHosting.md b/docs/ServiceHosting.md index cddf97ce..fa4c8740 100644 --- a/docs/ServiceHosting.md +++ b/docs/ServiceHosting.md @@ -32,7 +32,7 @@ Single-node dev: `OTOPCUA_ROLES=admin,driver` (or the `Cluster:Roles` equivalent ### Per-role configuration overlays -`Program.cs` builds a role suffix by joining the resolved `EffectiveRoles.Gossiped` roles **alphabetically** with `-` and loads `appsettings.{roleSuffix}.json` as an optional overlay on top of base `appsettings.json`. Three overlays ship in `src/Server/ZB.MOM.WW.OtOpcUa.Host/`: +`Program.cs` builds a role suffix by joining the resolved `EffectiveRoles.Gossiped` roles **alphabetically** with `-` and loads `appsettings.{roleSuffix}.json` as an optional overlay on top of base `appsettings.json`. A plain role (`admin`/`driver`/`dev`) is always lower-case in `Gossiped` regardless of how the operator spelled it, so the overlay filename for those is stable (`appsettings.driver.json`); if a `cluster-{ClusterId}` role is also configured, its suffix carries into the filename **verbatim** (e.g. `Cluster:Roles: ["driver", "cluster-SITE-A"]` loads `appsettings.cluster-SITE-A-driver.json`), since `Gossiped` preserves that spelling by design (see [Configuration.md § `Roles`](Configuration.md#appsettings-sections)). Three overlays ship in `src/Server/ZB.MOM.WW.OtOpcUa.Host/`: - `appsettings.admin.json` — admin-only nodes - `appsettings.driver.json` — driver-only nodes