dohertj2/lmxopcua
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

IRediscoverable + IHostConnectivityProbe events have no consumer in src/ (9 drivers raise into the void); CLAUDE.md Change Detection is wrong #518

New Issue
Closed
opened 2026-07-27 13:52:08 -04:00 by dohertj2 · 1 comment
dohertj2 commented 2026-07-27 13:52:08 -04:00
Owner
Copy Link
Copy Source

Found in the MTConnect Task 13 code review (PR #506) and verified independently against current master. Out of scope for that plan.

This is the "implemented but never dispatched" class that previously produced DeferredAddressSpaceSink and the gated-shut native alarms.

Verified

The only OnRediscoveryNeeded += / OnHostStatusChanged += subscriptions anywhere in src/ are GalaxyDriver.cs:586 and :221, where Galaxy wires its own sub-components (DeployWatcher / HostStatusAggregator) to re-raise on its own interface implementation. That is Galaxy fulfilling its contract — not an external consumer.

DriverInstanceActor wires exactly two driver events: OnDataChange (L721) and OnAlarmEvent (L792). DriverHostActor, GenericDriverNodeManager, AdminUI, ControlPlane and Cluster subscribe to neither of the two in question.

Impact

Modbus, S7, AbCip, AbLegacy, TwinCAT, FOCAS, OpcUaClient and Galaxy all raise these two events into the void. MTConnect now joins them. Every driver's rediscovery and host-connectivity work is dormant at the runtime layer.

Concretely for MTConnect: RediscoverPolicy=Once means an Agent restart should trigger a re-browse via OnRediscoveryNeeded. The driver raises it correctly (verified and mutation-tested in Task 13). Nothing listens. So a restarted Agent leaves a stale address space behind a Healthy driver until the next reconnect.

CLAUDE.md is factually wrong

The Change Detection section states that DeployWatcher "raises IRediscoverable.OnRediscoveryNeeded when the Galaxy redeploys. The server's DriverHost consumes the signal and rebuilds the address space."

DriverHost consumes nothing. Either the doc or the code needs to change.

Check before concluding the interface is dead

IHostConnectivityProbe.GetHostStatuses() — the polled half — may well have a live consumer via Configuration/Entities/DriverHostStatus.cs. Verify whether the polled path works even though the event path does not, before treating the whole interface as unused.

Suggested fix

Decide whether these events should be wired in DriverInstanceActor alongside OnDataChange/OnAlarmEvent, or whether the interfaces should be retired. Then reconcile CLAUDE.md with whichever answer wins.

Found in the MTConnect Task 13 code review (PR #506) and verified independently against current `master`. Out of scope for that plan. This is the **"implemented but never dispatched"** class that previously produced `DeferredAddressSpaceSink` and the gated-shut native alarms. ## Verified The **only** `OnRediscoveryNeeded +=` / `OnHostStatusChanged +=` subscriptions anywhere in `src/` are `GalaxyDriver.cs:586` and `:221`, where Galaxy wires its **own** sub-components (`DeployWatcher` / `HostStatusAggregator`) to re-raise on its own interface implementation. That is Galaxy fulfilling its contract — **not** an external consumer. `DriverInstanceActor` wires exactly two driver events: `OnDataChange` (`L721`) and `OnAlarmEvent` (`L792`). `DriverHostActor`, `GenericDriverNodeManager`, AdminUI, ControlPlane and Cluster subscribe to neither of the two in question. ## Impact Modbus, S7, AbCip, AbLegacy, TwinCAT, FOCAS, OpcUaClient and Galaxy all raise these two events **into the void**. MTConnect now joins them. Every driver's rediscovery and host-connectivity work is dormant at the runtime layer. Concretely for MTConnect: `RediscoverPolicy=Once` means an Agent restart should trigger a re-browse via `OnRediscoveryNeeded`. The driver raises it correctly (verified and mutation-tested in Task 13). Nothing listens. So a restarted Agent leaves a **stale address space behind a Healthy driver** until the next reconnect. ## CLAUDE.md is factually wrong The **Change Detection** section states that `DeployWatcher` "raises `IRediscoverable.OnRediscoveryNeeded` when the Galaxy redeploys. The server's `DriverHost` consumes the signal and rebuilds the address space." `DriverHost` consumes nothing. Either the doc or the code needs to change. ## Check before concluding the interface is dead `IHostConnectivityProbe.GetHostStatuses()` — the **polled** half — may well have a live consumer via `Configuration/Entities/DriverHostStatus.cs`. Verify whether the polled path works even though the event path does not, before treating the whole interface as unused. ## Suggested fix Decide whether these events should be wired in `DriverInstanceActor` alongside `OnDataChange`/`OnAlarmEvent`, or whether the interfaces should be retired. Then reconcile CLAUDE.md with whichever answer wins.
dohertj2 commented 2026-07-30 03:54:06 -04:00
Author
Owner
Copy Link
Copy Source

PARTIALLY RESOLVED — closing, with the unresolved half split out to #521. Read this before assuming both capabilities are now consumed.

IRediscoverable — RESOLVED (09a401b8, merged 914d70bf)

DriverInstanceActor now consumes IRediscoverable.OnRediscoveryNeeded (attach in PreStart, detach in PostStop) and carries the raise on the driver-health snapshot to the AdminUI /hosts page as a re-browse chip (DriverHealthChanged.RediscoveryNeededUtc / .RediscoveryReason).

⚠️ It is consumed as an operator PROMPT, not as an address-space rebuild. The served address space is deliberately unchanged: v3 authors raw tags explicitly through the /raw browse-commit flow, so a runtime graft would materialise nodes nobody approved and no deployment artifact records. This issue's framing ("CLAUDE.md Change Detection is wrong") was right that the doc lied; the fix was to make the doc true and give the signal a real consumer, not to build the rebuild the doc described.

Two traps pinned by tests, worth knowing before touching this:

  • PublishHealthSnapshot dedups on a health fingerprint, and a rediscovery raise changes none of the four fields it originally hashed — the timestamp had to join the tuple or the dedup swallows the signal on an otherwise-unchanged Healthy driver. Verified by reverting that one line: 3 of the 5 new tests go red.
  • The shared StubDriver.GetHealth() returns DateTime.UtcNow, so its fingerprint differs on every call and the dedup never engages. A dedup test built on it passes whether or not the fix is present; the new tests use a stub with stable health for exactly this reason.

The detach is load-bearing, not tidiness: the IDriver instance outlives the actor when the host respawns a child around the same driver object, so a missing -= accumulates one handler per respawn, each holding a dead Self.

IHostConnectivityProbe — NOT resolved, split to #521

GetHostStatuses() still has zero production call sites and nothing ever writes a DriverHostStatus row. It was not deleted on inference: the table was deliberately re-created in the v3 initial migration (20260715230637_V3Initial.cs:108), so removing it needs a decision rather than an assumption. #521 costs both options and notes that the separable OnHostStatusChanged event is the useful half.

Related, so it does not surprise anyone reading this later

A second change signal — a periodic 5-minute tag-set drift check for the four genuinely-browsable drivers — was built on top of this and then removed (#523, merge 2a0461a1). Consequence: AbCip and FOCAS browse a live backend but implement no IRediscoverable, so they have no tag-change signal at all. Galaxy, TwinCAT, MTConnect and MQTT raise natively and are unaffected by that removal.

**PARTIALLY RESOLVED — closing, with the unresolved half split out to #521.** Read this before assuming both capabilities are now consumed. ### `IRediscoverable` — RESOLVED (`09a401b8`, merged `914d70bf`) `DriverInstanceActor` now consumes `IRediscoverable.OnRediscoveryNeeded` (attach in `PreStart`, detach in `PostStop`) and carries the raise on the driver-health snapshot to the AdminUI `/hosts` page as a **re-browse chip** (`DriverHealthChanged.RediscoveryNeededUtc` / `.RediscoveryReason`). ⚠️ **It is consumed as an operator PROMPT, not as an address-space rebuild.** The served address space is deliberately unchanged: v3 authors raw tags explicitly through the `/raw` browse-commit flow, so a runtime graft would materialise nodes nobody approved and no deployment artifact records. This issue's framing ("CLAUDE.md Change Detection is wrong") was right that the doc lied; the fix was to make the doc true *and* give the signal a real consumer, not to build the rebuild the doc described. Two traps pinned by tests, worth knowing before touching this: - `PublishHealthSnapshot` dedups on a health fingerprint, and a rediscovery raise changes **none** of the four fields it originally hashed — the timestamp had to join the tuple or the dedup swallows the signal on an otherwise-unchanged Healthy driver. **Verified by reverting that one line: 3 of the 5 new tests go red.** - The shared `StubDriver.GetHealth()` returns `DateTime.UtcNow`, so its fingerprint differs on every call and the dedup never engages. A dedup test built on it passes **whether or not the fix is present**; the new tests use a stub with stable health for exactly this reason. The detach is load-bearing, not tidiness: the `IDriver` instance **outlives** the actor when the host respawns a child around the same driver object, so a missing `-=` accumulates one handler per respawn, each holding a dead `Self`. ### `IHostConnectivityProbe` — NOT resolved, split to #521 `GetHostStatuses()` still has **zero production call sites** and nothing ever writes a `DriverHostStatus` row. It was **not** deleted on inference: the table was deliberately re-created in the v3 initial migration (`20260715230637_V3Initial.cs:108`), so removing it needs a decision rather than an assumption. #521 costs both options and notes that the separable `OnHostStatusChanged` event is the useful half. ### Related, so it does not surprise anyone reading this later A **second** change signal — a periodic 5-minute tag-set drift check for the four genuinely-browsable drivers — was built on top of this and then **removed** (#523, merge `2a0461a1`). Consequence: **AbCip and FOCAS browse a live backend but implement no `IRediscoverable`, so they have no tag-change signal at all.** Galaxy, TwinCAT, MTConnect and MQTT raise natively and are unaffected by that removal.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
auto-managed
queue management
 cross-driver
queue management
 driver/abcip
queue management
 driver/ablegacy
queue management
 driver/focas
queue management
 driver/mqtt
MQTT / Sparkplug B broker-subscribe driver
 driver/opcuaclient
queue management
 driver/s7
queue management
 driver/twincat
queue management
 phase/1
queue management
 phase/2
queue management
 phase/3
queue management
 phase/4
queue management
 phase/5
queue management
 phase/6
queue management
 queue/blocked
queue management
 queue/done
queue management
 queue/failed
queue management
 queue/in-progress
queue management
 queue/queued
queue management
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
dohertj2 (dohertj2)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#518
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?