Merge branch 'master' into fix/r3-02-stale-good-family
# Conflicts: # src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs # tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Harness/V3ArtifactSeeder.cs
This commit is contained in:
+109
-1
@@ -164,6 +164,93 @@ mostly reusable — what was always missing is the trigger.
|
||||
|
||||
---
|
||||
|
||||
## What a (re)spawn re-provisions
|
||||
|
||||
A driver child can come into existence by **three** routes, and until 2026-08-20 only the first of them
|
||||
handed the child everything it needs to be a working driver. That is archreview **03/S16** (and its worse
|
||||
half, **03/S6**): a child that exists without its desired subscription set is a *dead* driver — it
|
||||
connects, reports `Connected`, publishes nothing and raises no alarms, and the #488 periodic reconcile
|
||||
cannot rescue it (it early-returns on an empty desired set). The fix makes the desired set **part of what
|
||||
it means to have a child** rather than a later step in the apply pipeline.
|
||||
|
||||
**`DriverHostActor._desiredByDriver` is the single authority.** It is a per-driver
|
||||
`(ValueRefs, AlarmRefs)` pair, cleared and repopulated **whole** on every apply in the same pass that
|
||||
rebuilds the routing maps — keyed over the union of the value-ref map, the alarm-ref map and the running
|
||||
`_children`, so a driver with no authored tags gets an explicit **empty** set rather than being absent.
|
||||
Everything that hands a child a `SetDesiredSubscriptions` goes through the one helper
|
||||
`SendDesiredSubscriptions(actor, driverId)`. (An empty set is meaningful, not a no-op to skip: the child
|
||||
routes it to `Unsubscribe`, dropping a stale handle.)
|
||||
|
||||
| Route | Trigger | What re-provisions it |
|
||||
|---|---|---|
|
||||
| **Deploy reconcile** | `ApplyDriverSpecs` → `SpawnChild` | `SpawnChild` sends `InitializeRequested` then the desired set; the apply's bulk push then sends the (freshly rebuilt) set to every child. |
|
||||
| **Operator Restart** | `/hosts` → Restart → `HandleRestartDriver` | Stops the child, respawns from the cached spec — and `SpawnChild` sends the desired set, because it always does. **This is the 03/S16 fix**: there is no bulk push on this path, and before it the respawned child subscribed to nothing until the next deployment. |
|
||||
| **Supervision restart** | a child throws ⇒ Akka's **default decider** ⇒ `Restart` | `DriverInstanceActor.PostRestart` Tells the host `ReprovisionRequested`; `HandleReprovisionRequested` logs a Warning naming the crash type, re-sends `InitializeRequested` (non-stub children) and the desired set. |
|
||||
| **Force Reconnect** | `/hosts` → Force Reconnect → `HandleReconnectDriver` | **Nothing — verified.** It Tells `ForceReconnect` to the *same* child actor, so the instance and its `_desiredRefs`/`_desiredAlarmRefs` survive and the reconnect's Connected entry re-applies them itself. Pinned by `Force_reconnect_keeps_the_desired_set_and_re_subscribes` so a future edit to the restart paths cannot break the one that was already correct. |
|
||||
|
||||
Things to know before touching any of this:
|
||||
|
||||
- ⚠️ **The supervision route is the invisible one, and it was the worst.** There is **no**
|
||||
`SupervisorStrategy` anywhere under `src/Server`, so a child that throws gets Akka's default `Restart`.
|
||||
A restart recreates the actor object around the **same** `IDriver` — the Props closure captured it —
|
||||
and its `PreRestart` runs `PostStop`, which calls `IDriver.ShutdownAsync`. It does **not** run
|
||||
`SpawnChild`, and the child's `IActorRef` is unchanged, so the host had no notification a new
|
||||
incarnation existed at all. Net effect before the hook: the child sat in `Connecting` forever around a
|
||||
shut-down driver, with its desired set gone, and nothing logged the non-recovery.
|
||||
- **Replacing the default decider is deliberately out of scope.** A `Stop` directive would silently
|
||||
remove a driver; a bounded retry count would silently stop one after N crashes. Both change failure
|
||||
semantics for every driver in the fleet and deserve their own plan and live gate. The reprovision hook
|
||||
makes the *current* infinite-restart behaviour correct instead of dead.
|
||||
- ⚠️ **…which is exactly why the reprovision is RATE-LIMITED.** The default decider restarts a faulting
|
||||
child **immediately and without limit**, and the hook now makes every one of those restarts do
|
||||
`ShutdownAsync` + `InitializeAsync` + `SubscribeAsync` — so a driver that faults on every value would
|
||||
hammer its device as fast as it can crash. `DriverHostActor` keeps a per-driver last-reprovision stamp
|
||||
and holds anything inside `DefaultReprovisionBackoff` (10 s, matched to
|
||||
`DriverInstanceActor.DefaultReconnectInterval` because it drives the same connect work). The held
|
||||
request is **deferred, not dropped** — one `StartSingleTimer` per driver, so a burst collapses to a
|
||||
single pending reprovision — because dropping it would leave a child whose *final* crash fell inside
|
||||
the window un-provisioned forever, which is the 03/S16 failure this hook exists to close. Each
|
||||
deferral logs a Warning naming the crash type, so a crash loop is visible rather than merely quiet.
|
||||
- **A reprovision is routed by INCARNATION, not by driver id.** `ReprovisionRequested` is delivered
|
||||
asynchronously, so one can still be in flight from an old incarnation after `HandleRestartDriver`
|
||||
stopped that child and spawned a replacement under the same id. Delivered to the new child — which is
|
||||
in `Connecting`, where `InitializeRequested` *is* handled — it would start a second, concurrent
|
||||
`InitializeAsync`, racing the dispose-before-overwrite that #527's teardown exists to serialise
|
||||
(`ModbusDriver._transport` / `MqttDriver._connection` each leak a transport on that race). The handler
|
||||
therefore checks `Sender` against the child it currently holds and drops anything else; the deferred
|
||||
path carries the original ref so the same check still applies when the timer re-delivers it.
|
||||
- **Ordering on a respawning deploy, and its one accepted cost.** `ApplyDriverSpecs` (which spawns) runs
|
||||
before the pass that rebuilds `_desiredByDriver`, so a child respawned by a deploy — raw tags are folded
|
||||
into the merged `DriverConfig`, so a tag-set edit changes the spec and trips the #516 config respawn —
|
||||
is first handed the **previous** apply's set, then the new one by the bulk push. Akka's per-pair
|
||||
ordering guarantees it **converges**; it does not guarantee the transient is inert, because the child
|
||||
runs concurrently with the rest of the apply handler and a fast-initialising driver can reach
|
||||
`Connected` and subscribe the stale set first. The cost is one extra subscribe round-trip
|
||||
(`HandleSubscribeAsync` drops the prior subscription before establishing the new one); a real device
|
||||
driver, whose initialise opens a socket, rarely wins that race. Withholding the spawn-time send to avoid
|
||||
it would simply restore 03/S16, since the Restart and supervision routes have no bulk push coming.
|
||||
- ⚠️ **Convergence is not free on the ALARM side, and "a subscription already exists" is the wrong guard
|
||||
there.** The value path converges by construction — `StoreDesiredSubscriptions` is a full replace and
|
||||
`HandleSubscribeAsync` drops the prior subscription before establishing the new one — but
|
||||
`SubscribeDesiredAlarms` / `HandleSubscribeAlarmsAsync` used to gate purely on
|
||||
`_alarmSubscriptionHandle is not null`, so once a handle existed the alarm set **latched for the life of
|
||||
the connection**. Combined with the transient above, a deploy widening a driver's alarm refs `{X}` →
|
||||
`{X, Y}` could leave a fast-initialising driver subscribed to `{X}` forever, and the drivers that filter
|
||||
their feed on the subscribed `sourceNodeIds` (OPC UA Client, AB CIP, FOCAS) would simply never deliver
|
||||
the new alarm — silently. The guard is now "the desired refs differ from the ones the live handle was
|
||||
established with" (`_subscribedAlarmRefs`), and a changed set **releases the old subscription before
|
||||
re-subscribing** so a filtering driver does not accumulate two overlapping ones. There is still
|
||||
deliberately no unsubscribe-on-**empty**: a removed alarm tag's condition node is torn down on the
|
||||
address-space rebuild and `ForwardNativeAlarm` drops any transition whose `ConditionId` no longer maps.
|
||||
- **Restart tests must assert a FUNCTION, never existence.** `RestartDriver_respawns_the_child_without_an_actor_name_collision`
|
||||
asserts only that a child exists after a restart, and it passed green against the dead driver 03/S16
|
||||
describes. It is kept — its own subject, the `InvalidActorNameException`, is real — but the load-bearing
|
||||
suite is `DriverHostActorRestartTests`, which asserts that the new incarnation *subscribed* and
|
||||
*re-initialised*. Its file remarks also record the supervision crash seam and why the obvious
|
||||
alternatives (a throwing health publisher, a throwing `SubscribeAsync`, `Kill.Instance`) do not work.
|
||||
|
||||
---
|
||||
|
||||
## IDriverHealthPublisher — health pub/sub sink
|
||||
|
||||
[`Core.Abstractions/IDriverHealthPublisher.cs`](../src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverHealthPublisher.cs)
|
||||
@@ -267,7 +354,8 @@ The `Connected`-state health tick now also evaluates a watchdog, and self-sends
|
||||
|
||||
1. the driver reports a state other than `Healthy`, **and**
|
||||
2. `DriverHealth.LastSuccessfulRead` has not advanced for `DefaultCommsLossTimeout` (90 s = three ticks),
|
||||
measured from the Connected entry when the driver has never reported a successful read.
|
||||
measured from **the later of that read stamp and the current Connected entry** (see "the grace is per
|
||||
connection" below).
|
||||
|
||||
**The conjunction is the whole design, and "non-Healthy for N polls" is the wrong rule.** `Degraded` means
|
||||
*connected but some data is bad* — the enum says so. Several drivers publish health **per tag inside their
|
||||
@@ -281,6 +369,26 @@ still needed as the second conjunct, because a `Healthy` driver with nothing sub
|
||||
reads and must not be declared dead for being idle. Both halves are pinned by their own test in
|
||||
`DriverInstanceActorCommsLossWatchdogTests`, each of which goes red when the *other* conjunct is deleted.
|
||||
|
||||
**The grace is per CONNECTION — the baseline is `max(LastSuccessfulRead, connectedSince)`, not
|
||||
`LastSuccessfulRead ?? connectedSince`** (archreview 03/S17, fixed 2026-08-20). The two spellings differ
|
||||
for exactly the case that matters, and the `??` one collapsed the grace to a single health tick. Because
|
||||
`LastSuccessfulRead` is *preserved* across `Degraded`/`Faulted` writes — that preservation is the design
|
||||
described above — a device that accepts a TCP connection but never serves data (a half-broken PLC, a wrong
|
||||
unit id, a firewalled read path) reconnects carrying a read stamp frozen from before the outage, already
|
||||
far older than the threshold. Measuring from that stamp alone re-trips on the **first** tick of every new
|
||||
connection, so the reconnect cycle ran at health-tick cadence (~30 s in production) rather than at the
|
||||
designed 90 s. Taking the *later* of the two gives each connection its own full window, which is what
|
||||
re-stamping `connectedSince` on every Connected entry was always for; `??` only ever helped a driver that
|
||||
had never read at all, and the max still covers that case.
|
||||
|
||||
What this does **not** change: the conjunction, the disable-at-zero hatch, and a driver that stops reading
|
||||
*within* a stable connection (its read stamp is the later term, so it still trips on schedule measured
|
||||
from that read). And it **paces** the half-broken-device cycle rather than ending it — expect at most one
|
||||
reconnect attempt per threshold per connection, not silence. Pinned by
|
||||
`A_reconnected_driver_gets_a_full_grace_window_before_the_watchdog_trips_again` (the delay) and
|
||||
`A_half_broken_device_is_still_declared_lost_once_the_grace_elapses` (the anti-vacuity half — without it,
|
||||
a "fix" that simply disabled the watchdog after the first connection would look correct).
|
||||
|
||||
Two consequences worth knowing:
|
||||
|
||||
- **`CalculationDriver` can never trip it, correctly.** It reports `Healthy` whenever it is running and has
|
||||
|
||||
Reference in New Issue
Block a user