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

Driver config edits are silently discarded on Modbus/FOCAS/OpcUaClient (deployment still seals green) #516

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

Found while building the MTConnect driver (PR #506) and verified independently end-to-end against current master. Deliberately not fixed there — out of scope for that plan.

Symptom

An operator edits a Modbus / FOCAS / OpcUaClient driver's config and deploys. The driver keeps running the old config. ApplyResult(true) is returned and the deployment seals green.

Silent no-op — the worst shape, because it looks successful.

Cause

DriverInstanceActor assigns _driver once (DriverInstanceActor.cs:307) and never rebuilds it from the factory. Both config-change paths therefore hand the new JSON to that already-constructed instance:

  • HandleApplyDeltaAsync (L592) → _driver.ReinitializeAsync(msg.DriverConfigJson, …)
  • AdoptConfigDuringInit → the actor's InitializeAsync (L554) → _driver.InitializeAsync(driverConfigJson, …)

So driverConfigJson is the only channel by which a config change can reach a running driver.

But these three never read that parameter — they use the ctor-injected _options exclusively:

  • ModbusDriver.InitializeAsync (L195)
  • FocasDriver.InitializeAsync (L104)
  • OpcUaClientDriver.InitializeAsync (L160)

Their ReinitializeAsync delegates to InitializeAsync, so the new config is dropped on the floor.

Prior art in-tree

S7 does it correctly and documents why — see the comment at S7Driver.cs:283. MTConnect follows S7, and this was confirmed live on the PR #506 gate: a corrected tag config took effect on redeploy with no container restart.

Still to check

The per-driver survey was inconclusive for AbCip, TwinCAT and Galaxy — verify each one's InitializeAsync actually re-parses before closing this.

Suggested fix

Make InitializeAsync re-parse driverConfigJson following S7's pattern (an empty/blank body keeps the ctor options). Add a per-driver regression test: reinitialise with a changed config and assert the change is observable, not merely that the call returned true.

Found while building the MTConnect driver (PR #506) and verified independently end-to-end against current `master`. Deliberately **not** fixed there — out of scope for that plan. ## Symptom An operator edits a Modbus / FOCAS / OpcUaClient driver's config and deploys. The driver keeps running the **old** config. `ApplyResult(true)` is returned and the deployment seals **green**. Silent no-op — the worst shape, because it looks successful. ## Cause `DriverInstanceActor` assigns `_driver` **once** (`DriverInstanceActor.cs:307`) and never rebuilds it from the factory. Both config-change paths therefore hand the new JSON to that already-constructed instance: - `HandleApplyDeltaAsync` (`L592`) → `_driver.ReinitializeAsync(msg.DriverConfigJson, …)` - `AdoptConfigDuringInit` → the actor's `InitializeAsync` (`L554`) → `_driver.InitializeAsync(driverConfigJson, …)` So `driverConfigJson` is the **only** channel by which a config change can reach a running driver. But these three never read that parameter — they use the ctor-injected `_options` exclusively: - `ModbusDriver.InitializeAsync` (`L195`) - `FocasDriver.InitializeAsync` (`L104`) - `OpcUaClientDriver.InitializeAsync` (`L160`) Their `ReinitializeAsync` delegates to `InitializeAsync`, so the new config is dropped on the floor. ## Prior art in-tree **S7 does it correctly and documents why** — see the comment at `S7Driver.cs:283`. MTConnect follows S7, and this was confirmed live on the PR #506 gate: a corrected tag config took effect on redeploy with no container restart. ## Still to check The per-driver survey was **inconclusive for AbCip, TwinCAT and Galaxy** — verify each one's `InitializeAsync` actually re-parses before closing this. ## Suggested fix Make `InitializeAsync` re-parse `driverConfigJson` following S7's pattern (an empty/blank body keeps the ctor options). Add a per-driver regression test: reinitialise with a changed config and assert the change is **observable**, not merely that the call returned true.
dohertj2 commented 2026-07-30 03:53:35 -04:00
Author
Owner
Copy Link
Copy Source

RESOLVED — fixed for all 12 drivers. Merged to master in 914d70bf (seam respawn + per-driver re-parse, d32d89c3; Sql/FOCAS atomic re-parse, 5184a2e1).

The survey in this issue was incomplete

It named Modbus, FOCAS and OpcUaClient. The defect also affected AbLegacy (missed here) and Sql (where discarding was deliberate and documented — on a premise that turned out to be false). AbCip and TwinCAT already re-parsed correctly; Galaxy fails loud (throws NotSupportedException) rather than silently.

Two halves, because reading the drivers split them

  • Re-parse in place — Modbus, AbLegacy, OpcUaClient. ParseOptions extracted from each factory and called from InitializeAsync behind a HasConfigBody guard, so "{}" still means "keep the constructor options".
  • Atomic re-parse — Sql and FOCAS derive more than options from config (Sql's ISqlDialect + resolved connection string; FOCAS's client-factory backend, injected at construction). Adopting new options alone would run a new tag set against an old connection — a half-applied change is worse than a discarded one, because it looks like it worked. Each factory now exposes a ParseBinding returning every config-derived dependency as one value, adopted together.

The load-bearing half was the seam, not the drivers

DriverSpawnPlanner now routes a changed DriverConfig to ToStop + ToSpawn, making the factory the single parse authority. This reverses the deliberate decision documented at DriverSpawnPlan.cs:49-50 ("a pure DriverConfig change stays an in-place delta — no reconnect"). That reasoning was right about resilience and wrong about the driver.

⚠️ Accepted, operationally visible price: a config edit now costs a driver reconnect.

Two green seals were removed from ApplyChildDelta en route: it overwrote the cached Spec synchronously, before the child had dequeued the message, so the next reconcile computed no delta and sealed the drift permanently; and it Telld with no Receive<ApplyResult> registered, so a failed reinit (including Galaxy's deliberate throw) dead-lettered.

Why this survived review for so long

Every pre-existing reinit test in the five suites passed "{}" — precisely the input a guarded re-parser treats as "keep the constructor options". They were blind to this defect by construction. The new tests pass changed JSON, and the Modbus one was verified falsifiable by deleting the re-parse line and watching it go red. The Sql/FOCAS tests pin atomicity, not merely "a re-parse happened" — verified by simulating the half-fix (adopt options, skip the backend): 2 of the 3 FOCAS tests go red.

Side benefit worth knowing

A rotated credential is now picked up without a process restart, because the driver re-resolves connectionStringRef when it re-parses.

**RESOLVED** — fixed for **all 12 drivers**. Merged to `master` in `914d70bf` (seam respawn + per-driver re-parse, `d32d89c3`; Sql/FOCAS atomic re-parse, `5184a2e1`). ### The survey in this issue was incomplete It named Modbus, FOCAS and OpcUaClient. The defect also affected **AbLegacy** (missed here) and **Sql** (where discarding was deliberate and documented — on a premise that turned out to be false). AbCip and TwinCAT already re-parsed correctly; Galaxy fails **loud** (throws `NotSupportedException`) rather than silently. ### Two halves, because reading the drivers split them - **Re-parse in place** — Modbus, AbLegacy, OpcUaClient. `ParseOptions` extracted from each factory and called from `InitializeAsync` behind a `HasConfigBody` guard, so `"{}"` still means "keep the constructor options". - **Atomic re-parse** — Sql and FOCAS derive *more than options* from config (Sql's `ISqlDialect` + resolved connection string; FOCAS's client-factory backend, injected at construction). Adopting new options alone would run a **new tag set against an old connection** — a half-applied change is worse than a discarded one, because it looks like it worked. Each factory now exposes a `ParseBinding` returning every config-derived dependency as one value, adopted together. ### The load-bearing half was the seam, not the drivers `DriverSpawnPlanner` now routes a changed `DriverConfig` to `ToStop` + `ToSpawn`, making the factory the single parse authority. This **reverses the deliberate decision documented at `DriverSpawnPlan.cs:49-50`** ("a pure DriverConfig change stays an in-place delta — no reconnect"). That reasoning was right about resilience and wrong about the driver. ⚠️ **Accepted, operationally visible price: a config edit now costs a driver reconnect.** Two green seals were removed from `ApplyChildDelta` en route: it overwrote the cached `Spec` **synchronously, before the child had dequeued the message**, so the next reconcile computed no delta and sealed the drift permanently; and it `Tell`d with no `Receive<ApplyResult>` registered, so a failed reinit (including Galaxy's deliberate throw) dead-lettered. ### Why this survived review for so long **Every pre-existing reinit test in the five suites passed `"{}"`** — precisely the input a guarded re-parser treats as "keep the constructor options". They were blind to this defect *by construction*. The new tests pass **changed** JSON, and the Modbus one was verified falsifiable by deleting the re-parse line and watching it go red. The Sql/FOCAS tests pin **atomicity**, not merely "a re-parse happened" — verified by simulating the half-fix (adopt options, skip the backend): 2 of the 3 FOCAS tests go red. ### Side benefit worth knowing A **rotated credential is now picked up without a process restart**, because the driver re-resolves `connectionStringRef` when it re-parses.
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#516
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?