From 12c402847411d7a709fdf16986cd9e90467742bf Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:07:20 -0400 Subject: [PATCH] docs: restart re-provisioning + per-connection watchdog grace (R3-03) --- ...R3-03-restart-resubscribe-watchdog-plan.md | 13 +++ ...rt-resubscribe-watchdog-plan.md.tasks.json | 2 +- docs/DriverLifecycle.md | 79 ++++++++++++++++++- 3 files changed, 92 insertions(+), 2 deletions(-) diff --git a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md index 4b50050c..f8043aa9 100644 --- a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md +++ b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md @@ -640,3 +640,16 @@ rather than `PresenceBudget`, because `SpawnChild` gives the child the productio sooner; a budget is only spent on failure. After restoring the hook: **6/6 pass**, and the class was run **20× in isolation, 20/20 green**. + +### T8 — docs + +`docs/DriverLifecycle.md`: + +- New §"What a (re)spawn re-provisions" (placed before §IDriverHealthPublisher): `_desiredByDriver` as + the single authority, a table of the three incarnation routes plus the verified-negative Force + Reconnect, the supervision route's invisibility, why replacing the default decider is out of scope, + the respawning-deploy ordering **with the measured caveat that the transient is not inert**, and the + assert-a-function-not-existence rule naming the weaker existing test. +- §"The comms-loss watchdog": conjunct 2 restated as the max, plus a new paragraph on the + per-connection grace — why `??` was wrong, that it **paces** rather than ends the half-broken-device + cycle, that the watchdog is not disabled by the change, and the two tests that pin each half. diff --git a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json index 625327b6..5e1d2dab 100644 --- a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json +++ b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json @@ -57,7 +57,7 @@ { "id": "T8", "subject": "docs/DriverLifecycle.md: what a (re)spawn re-provisions + per-connection watchdog grace", - "status": "pending", + "status": "done", "blockedBy": [] }, { diff --git a/docs/DriverLifecycle.md b/docs/DriverLifecycle.md index 3e4519db..c09fafce 100644 --- a/docs/DriverLifecycle.md +++ b/docs/DriverLifecycle.md @@ -164,6 +164,62 @@ 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. +- **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. +- **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) @@ -236,7 +292,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 @@ -250,6 +307,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