From 6c81c0bc0a4e9c4315eaffec2c816cd9ea15cbd3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:43:18 -0500 Subject: [PATCH] fix(overview): resolve a group's leader against its own members (scadaproj#43) LeaderResolver searched every instance of the application, so co-located groups (QA: ScadaBridge central-a and site-a both on vq03) got the first-listed group's instance name on their leader chip. Resolution is now scoped to the group's members; an unmatched address keeps its raw form. - tests: QA co-located central/site layout; cross-group host not borrowed - CLAUDE.md: correct the leader-resolution / 127.0.0.1 guidance - code-reviews: Overview-002 Resolved; README regenerated Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.Overview/CLAUDE.md | 20 +++++------ .../Polling/LeaderResolver.cs | 14 ++++---- .../LeaderResolverTests.cs | 34 +++++++++++++++++++ code-reviews/Overview/findings.md | 6 ++-- code-reviews/README.md | 11 +++--- 5 files changed, 60 insertions(+), 25 deletions(-) diff --git a/ZB.MOM.WW.Overview/CLAUDE.md b/ZB.MOM.WW.Overview/CLAUDE.md index c809b1e..1531c5d 100644 --- a/ZB.MOM.WW.Overview/CLAUDE.md +++ b/ZB.MOM.WW.Overview/CLAUDE.md @@ -30,21 +30,21 @@ service `ZbOverview` on **:5320** under the shared NSSM, `ASPNETCORE_ENVIRONMENT - **Publish self-contained** (`-r win-x64 --self-contained true`). wonder has **no .NET 10 runtime** — every app there carries its own. An FDD publish installs fine and then fails at service start. -- **Registry `BaseUrl` must not put co-located instances on a shared host name.** Leader-address → - instance resolution matches on **host only** (Akka and HTTP ports differ by design), which assumes - distinct hosts per instance — true on the rig, false on a single box, where the fleet-wide scan - lets the first matching entry win the chip for *every* group. On wonder that made ScadaBridge's - **site** card claim `Leader · central`, a different Akka cluster. Fixed by using `127.0.0.1`, - which matches nothing, so each chip falls back to the raw Akka address. +- **Leader resolution is per group (fixed 2026-11-09, Overview-002 / scadaproj#43).** Leader-address → + instance resolution matches on **host only** (Akka and HTTP ports differ by design). It used to scan + the whole application, so co-located groups stole each other's chip — on vd03 ScadaBridge's **site** + card claimed `Leader · central`, and the same happened on QA, where central-a and site-a both run on + vq03 (the earlier note that QA "resolves as designed" was wrong). It now searches only the group's + own members and keeps the raw Akka address when none matches. The vd03 `127.0.0.1` BaseUrls are no + longer needed for correctness (each vd03 group is single-node); they remain harmless. Two members + of the **same** group on one host would still be indistinguishable by host. **QA runs an instance on EACH pair node — `wonder-app-vq03:5320` and (since 2026-08-14) `wonder-app-vq04:5320`** — both watching the same registry, now **10 instances across 4 applications** after the 2026-08-14 VQ04 singleton clone (MxGateway + HistorianGateway added on vq04; see `deployment.md` §7.1). It is the fastest way to read QA's whole topology. -Its registry uses **real FQDNs**, and that is correct rather than a divergence: the `127.0.0.1` -rule above exists only because VD03 co-locates every instance on one host. QA is a **2-node pair** -(`vq03`/`vq04`) with genuinely distinct hosts, so leader resolution works as designed and the chips -resolve to real names (`node-a (vq03)`). Carry the *reason*, not the literal `127.0.0.1`. +Its registry uses **real FQDNs**. Each QA group's two members are on distinct hosts (`vq03`/`vq04`), +which per-group resolution needs; groups sharing a host no longer matter (see above). ## Commands (from the index) diff --git a/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/LeaderResolver.cs b/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/LeaderResolver.cs index 79f751a..aa7dba4 100644 --- a/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/LeaderResolver.cs +++ b/ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/LeaderResolver.cs @@ -45,16 +45,13 @@ public static class LeaderResolver if (!members.Exists(m => m.HasActiveRole)) continue; - groups.Add(ResolveGroup(name!, members, all)); + groups.Add(ResolveGroup(name!, members)); } return groups; } - private static GroupSnapshot ResolveGroup( - string name, - IReadOnlyList members, - IReadOnlyList allInstances) + private static GroupSnapshot ResolveGroup(string name, IReadOnlyList members) { // Only members that are actually talking get a vote. A Down or Unreachable node's last // known leader is stale by definition, and counting it would manufacture a disagreement @@ -80,9 +77,14 @@ public static class LeaderResolver .First() .Key; + // Resolved against the group's OWN members only. A group's leader is one of its members by + // definition, and co-located groups of one application (QA: ScadaBridge central-a and + // site-a both on vq03) share hosts — searching the whole application let the first-listed + // group's instance win every later group's chip (Overview-002). An unmatched address keeps + // its raw form rather than borrowing a name from another cluster. return new GroupSnapshot( name, - ResolveToInstanceName(winner, allInstances), + ResolveToInstanceName(winner, members), winner, Disagreement: distinct.Count > 1, distinct); diff --git a/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/LeaderResolverTests.cs b/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/LeaderResolverTests.cs index 8704dc5..8538081 100644 --- a/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/LeaderResolverTests.cs +++ b/ZB.MOM.WW.Overview/tests/ZB.MOM.WW.Overview.Tests/LeaderResolverTests.cs @@ -171,6 +171,40 @@ public class LeaderResolverTests Assert.All(groups, g => Assert.False(g.Disagreement)); } + [Fact] + public void Resolve_CoLocatedGroups_ResolveTheLeaderWithinTheirOwnGroup() + { + // Overview-002 (scadaproj#43): the QA layout — ScadaBridge central-a and site-a both on + // vq03, central-b and site-b on vq04, all in ONE application. Resolving against every + // instance named the site group's leader "central-a" (a node in another Akka cluster). + var groups = LeaderResolver.Resolve( + [ + Snapshots.Instance("central-a", "central", "http://vq03:9000", leader: "akka.tcp://scadabridge@vq03:8081"), + Snapshots.Instance("central-b", "central", "http://vq04:9000", leader: "akka.tcp://scadabridge@vq03:8081"), + Snapshots.Instance("site-a", "site", "http://vq03:8084", leader: "akka.tcp://scadabridge@vq03:8082"), + Snapshots.Instance("site-b", "site", "http://vq04:8084", leader: "akka.tcp://scadabridge@vq03:8082"), + ]); + + Assert.Equal("central-a", groups.Single(g => g.Name == "central").LeaderInstance); + Assert.Equal("site-a", groups.Single(g => g.Name == "site").LeaderInstance); + } + + [Fact] + public void Resolve_LeaderHostOnlyInAnotherGroup_IsNotAttributedAcrossGroups() + { + // A leader that matches no member of its own group keeps its raw address rather than + // borrowing a name from a different cluster that happens to share the host. + var groups = LeaderResolver.Resolve( + [ + Snapshots.Instance("central-a", "central", "http://shared-host:9000", leader: "akka.tcp://scadabridge@shared-host:8081"), + Snapshots.Instance("site-a", "site", "http://127.0.0.1:8084", leader: "akka.tcp://scadabridge@shared-host:8082"), + ]); + + var site = groups.Single(g => g.Name == "site"); + Assert.Null(site.LeaderInstance); + Assert.Equal("akka.tcp://scadabridge@shared-host:8082", site.LeaderAddress); + } + [Theory] [InlineData("akka.tcp://scadabridge@site-a-a:8082", "site-a-a")] [InlineData("akka.tcp://otopcua@10.100.0.35:4053", "10.100.0.35")] diff --git a/code-reviews/Overview/findings.md b/code-reviews/Overview/findings.md index bbf5b8c..bf6a67c 100644 --- a/code-reviews/Overview/findings.md +++ b/code-reviews/Overview/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude | | Commit reviewed | `91fb2b4` | -| Open findings | 4 | +| Open findings | 3 | ## Summary @@ -134,7 +134,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#29)` — `ZbHealthReportCli | Severity | Medium | | Priority | P2 | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/LeaderResolver.cs:85` (scan at `:150-166`) | **Description** @@ -174,7 +174,7 @@ matches. Add a `LeaderResolverTests` case with two co-located groups. Then revis **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#43)` — `LeaderResolver.ResolveGroup` now resolves the winning address against the group's own members only. Deliberately **no** fall-back to the whole application (a deviation from the recommendation): the fall-back reintroduces the bug whenever a group's member does not match by host but another group's does (e.g. a `127.0.0.1` member next to an FQDN member of another group); an unmatched address keeps its raw form, as before. Regression tests `LeaderResolverTests.Resolve_CoLocatedGroups_ResolveTheLeaderWithinTheirOwnGroup` (the QA central/site layout; failed with `site` → `central-a`) and `Resolve_LeaderHostOnlyInAnotherGroup_IsNotAttributedAcrossGroups` both fail without the fix. `ZB.MOM.WW.Overview/CLAUDE.md` corrected; the matching `deployment.md` §7 note (root doc, outside this change) is still stale. ### Overview-003 — Status-only active probe treats any 200, including a followed redirect to a login page, as Active diff --git a/code-reviews/README.md b/code-reviews/README.md index 21bdd6a..86ee6e0 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 48 pending findings across all libraries. +10 of 10 libraries reviewed. 47 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -50,9 +50,9 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 15 | +| Medium | 14 | | Low | 33 | -| **Total** | **48** | +| **Total** | **47** | ## Library Status @@ -64,7 +64,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/2/6 | 8 | 8 | | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/1 | 1 | 7 | | [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | -| [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/3 | 4 | 5 | +| [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 5 | | [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/6/13 | 19 | 19 | | [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 11 | | [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/1 | 2 | 8 | @@ -84,7 +84,7 @@ _None open._ _None open._ -### Medium (15) +### Medium (14) | ID | Library | Title | |----|---------|-------| @@ -95,7 +95,6 @@ _None open._ | LocalDb-002 | [LocalDb](LocalDb/findings.md) | Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good | | LocalDb-003 | [LocalDb](LocalDb/findings.md) | `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges | | LocalDb-004 | [LocalDb](LocalDb/findings.md) | `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently | -| Overview-002 | [Overview](Overview/findings.md) | Leader address is resolved against every instance of the application, not the group, so co-located groups steal each other's chip | | Secrets-001 | [Secrets](Secrets/findings.md) | Calling `AddZbSecrets` before a SQL-Server extension silently cancels the SQL-Server store / hub replicator | | Secrets-002 | [Secrets](Secrets/findings.md) | The SQL-Server extensions cannot pass the content root, so the pre-host store-path hole reopens in hub mode | | Secrets-003 | [Secrets](Secrets/findings.md) | `${secret:}` expansion is undone by configuration reload |