From 5fd65d190e3734a116abfba63c19c9d87669eee3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:28:03 -0400 Subject: [PATCH] docs(archreview): round-3 plan R3-03 (restart re-subscribe + watchdog grace); index task count --- archreview/plans/00-INDEX.md | 2 +- ...R3-03-restart-resubscribe-watchdog-plan.md | 454 ++++++++++++++++++ ...rt-resubscribe-watchdog-plan.md.tasks.json | 75 +++ 3 files changed, 530 insertions(+), 1 deletion(-) create mode 100644 archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md create mode 100644 archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json diff --git a/archreview/plans/00-INDEX.md b/archreview/plans/00-INDEX.md index ba631d98..650073fb 100644 --- a/archreview/plans/00-INDEX.md +++ b/archreview/plans/00-INDEX.md @@ -22,7 +22,7 @@ co-located `.tasks.json`. |---|---|---|---|---|---|---| | 1 | [R3-01 Mesh gRPC `.AllowAnonymous()`](R3-01-mesh-grpc-allowanonymous-plan.md) | 03/S14 **Critical** | 10 | opus | sonnet (+ fable for the `DisableLogin=false` rig gate) | opus | | 2 | [R3-02 Stale-Good family](R3-02-stale-good-family-plan.md) | 03/S15, 05/STAB-19, 05/STAB-18, 02/S14 | TBD | **fable** | opus | opus | -| 3 | [R3-03 Restart re-subscribe + watchdog `max()`](R3-03-restart-resubscribe-watchdog-plan.md) | 03/S16, S17, U6 | TBD | opus | sonnet | opus | +| 3 | [R3-03 Restart re-subscribe + watchdog `max()`](R3-03-restart-resubscribe-watchdog-plan.md) | 03/S16, S17, U6 | 9 | opus | opus (upgraded from sonnet: the plan redesigns the spawn path + adds a supervision-restart hook) | opus | | 4 | [R3-04 One role authority](R3-04-one-role-authority-plan.md) | 01/S-12 = 03/C5, 01/C-9 | 13 | opus | sonnet | opus | | 5 | [R3-05 ACL hot path + key expiry](R3-05-acl-hot-path-expiry-plan.md) | 03/S18, S19 | TBD | opus | opus | opus | diff --git a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md new file mode 100644 index 00000000..e872aefd --- /dev/null +++ b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md @@ -0,0 +1,454 @@ +# Design + Implementation Plan — R3-03 Restart / Re-subscribe / Watchdog Grace + +- **Source report:** `archreview/03-server-runtime.md` §S16 · §S17 · §U6 (2026-08-19 round-3 re-review at `12b0035a`) +- **Prior designs consumed:** none — S16 and S17 are **new** this round; S6 ("driver-child restart loses desired-subscription state; no backoff supervision") is the carried finding S16 is the operator-visible face of, and its remaining exposure (the *supervision* restart) is brought into scope here rather than deferred a fourth round. +- **Plan verified against tree at:** master `7c77a8c7` (clean; code identical to `12b0035a`) — every anchor re-opened and re-checked 2026-08-20. +- **Scope:** OVERALL prioritized action item **#3** — "`HandleRestartDriver` re-pushes desired subscriptions after `SpawnChild` (consume `_lastComposition`); add the first actor-restart test; fix the watchdog `??` → `max(...)`" (03 S16, 03 S17, 03 U6). +- **Cross-cutting requirement (OVERALL theme 3, "built-but-never-wired" re-grown):** the finding is not merely "a restart forgets to subscribe" — it is that a *second* spawn path was added without the payload the *first* one sends, and a field (`_lastComposition`) was landed to carry that payload and then never read. A fix that adds a third call site to the same forgettable list re-creates the shape. The design therefore makes the desired set a **single authority consumed by `SpawnChild` itself**, so deploy-reconcile, operator-Restart and supervision-restart are one path, and deletes the write-only field rather than finally reading it. +- **Sibling-plan ordering — LAND THIS FIRST.** R3-02 (03/S15, stale-Good on comms loss) edits `DriverHostActor.OnDriverConnectivityChanged` (`:1248-1283`) and `FanCommsLossToDependencyMux` (`:1334-1359`). This plan edits `:347-349`, `:2596-2711`, `:2713-2800`, `:2916-2932` and the three `Receive` blocks at `:1078` / `:1133` / `:1715` — **no overlapping hunks**, but the same 3,063-line file. R3-03 is the smaller change with the cheaper tests; land it first and rebase R3-02 on it. + +## Verification summary + +All three findings **CONFIRMED at `7c77a8c7`** — none stale, none partially fixed. Two of the three anchors are exact to the line. Anchor drift status: + +| Finding | Report anchor | Status at `7c77a8c7` | +|---|---|---| +| 03/S16 | `DriverHostActor.cs:2916-2932` (`HandleRestartDriver`) | **Exact.** `:2916` signature; `:2920-2921` Info log; `:2925-2927` `StopDependencyAdapter` + `Context.Stop` + `_children.Remove`; `:2931` `SpawnChild(entry.Spec);` is the last statement. No `SetDesiredSubscriptions`, no `InitializeRequested` re-push (the latter is sent *inside* `SpawnChild` at `:2779`, so the restart path does re-initialise — only the subscription set is lost). | +| 03/S16 | `DriverHostActor.cs:2660-2672` ("the only sender") | **Exact, with one refinement**: the single `Tell` is `:2664`, inside the local function `SendAuthoredOnly` (`:2660-2666`), driven by the bulk loop `:2668-2672`. Repo-wide grep confirms `:2664` is the **only** `SetDesiredSubscriptions` send in `src/`. | +| 03/S16 | `_lastComposition` declared `:349`, assigned `:2709`, read nowhere | **Exact.** Two occurrences in the entire repo (`:349`, `:2709`); zero reads. Bonus drift: its xmldoc (`:347-348`) says "set at the END of `PushDesiredSubscriptions`" — the assignment is in `ApplyDesiredSubscriptions`, a different method (theme 6). | +| 03/S16 | `DriverInstanceActor.cs:625` (reconcile can't help) | **Exact.** `ReconcileSubscription` (`:622-631`) returns early on `_desiredRefs.Count == 0` (`:625`) — and on `_driver is not ISubscribable` (`:624`). A fresh child's `_desiredRefs` is `Array.Empty()` (`:241`), so the #488 reconcile is structurally inert for it, exactly as reported. | +| 03/S16 (extension) | supervision restart — *not* covered by the report's anchors | **CONFIRMED WORSE, new sub-finding.** Zero `SupervisorStrategy` / `OneForOneStrategy` / `Directive.` occurrences anywhere under `src/Server` (grep-verified — S6's other half), so `DriverHostActor` runs Akka's **default** decider: a plain `Exception` from a child ⇒ `Restart`. A restart recreates the actor object via the **same Props closure**, which captured the **same `IDriver` instance** (`Props.Create(() => new DriverInstanceActor(driver, …))`, `:288-296`) — so on restart: `PreRestart` → default → `PostStop` (`:1320-1329`) calls `IDriver.ShutdownAsync` and detaches everything; the new instance's ctor `Become(Connecting)`; `PostRestart` → `PreStart` (`:365-376`) re-attaches the rediscovery/host-status sources and restarts the health timer — **and nothing ever re-sends `InitializeRequested`** (its only sender is `SpawnChild:2779`, and `_children[id].Actor` is unchanged by a restart, so `SpawnChild` does not run). Net: the restarted child sits in `Connecting` forever around a **shut-down driver object**, with `_desiredRefs` empty. Strictly worse than S16, and equally invisible. | +| 03/S16 (extension) | Force Reconnect path | **NOT AFFECTED — verified negative.** `HandleReconnectDriver` (`:2935-2946`) Tells `ForceReconnect` to the **same** child actor; the instance survives, so `_desiredRefs`/`_desiredAlarmRefs` are intact, and the reconnect's success path calls `ResubscribeDesired()` + `SubscribeDesiredAlarms()` (`:657-659`). No fix needed; a regression test is added (T7) so a future "fix" to the restart path cannot break it. | +| 03/S17 | `DriverInstanceActor.cs:574` (`??`) | **Exact.** `var lastEvidence = observed.LastSuccessfulRead ?? _connectedSinceUtc;`. | +| 03/S17 | comment `:458-459` describing `max(...)` | **Exact.** "Re-stamped on every Connected entry so a driver that reconnects but never manages a read is measured from THIS connect, not from a stale earlier one" — `_connectedSinceUtc = DateTime.UtcNow;` at `:460`. | +| 03/S17 | "conjunction at `:571-576`" | **Minor drift (−1 at the top).** The guards are `:569` (timeout ≤ 0 disables), `:570` (null observation), `:571` (state conjunct), `:574-576` (silence conjunct). Use `:567-583` for the method. | +| 03/U6 | "no actor supervision/restart test anywhere" | **CONFIRMED, and the one restart test that exists is the vacuity the finding warns about.** `DriverHostActorReconcileTests.RestartDriver_respawns_the_child_without_an_actor_name_collision` (`:186-217`) asserts only *no `InvalidActorNameException`*, `factory.CreateCount == 2` and *one child remains* — i.e. that the child **exists**. It passes today against the driver that S16 leaves dead. Repo-wide, no test provokes a supervision restart of any actor. | + +**Supporting facts verified.** +`RuntimeActorTestBase` (`tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Harness/RuntimeActorTestBase.cs`) runs a real single-node Akka cluster (`ClusterActorRefProvider`, roles `["driver"]`, `seed-nodes = []`) and publishes `PresenceBudget = 15 s` as the shared **presence** budget, with the explicit rule that **absence windows keep their own short, individually-calibrated literals** (#500). +`StubDriver.GetHealth()` returns `new(DriverState.Healthy, DateTime.UtcNow, null)` — permanently Healthy with an always-fresh read stamp — and its own xmldoc (`StubDrivers.cs:66-73`) states that a watchdog test built on it is vacuous; the watchdog suite therefore uses `HealthControlledStubDriver` (`DriverInstanceActorCommsLossWatchdogTests.cs:263-330`), which is the **stable-health stub** this plan's S17 tests reuse unchanged (it already exposes `Health`, `HealthFactory`, `HealthReadCount`, `ObservedSpan`, `Lifecycle`, `InitializeCount`). +`SubscribableStubDriver` (`StubDrivers.cs:102-183`) already counts `SubscribeCount` and records `LastSubscribedRefs` — the observables the S16 falsification needs — but hard-codes `DriverInstanceId => "stub-driver-1"` on the non-virtual `StubDriver` member, so a per-instance recording driver is a new (small) test type rather than a subclass. +`V3ArtifactSeeder.SeedV3Deployment` (`Harness/V3ArtifactSeeder.cs:40-107`) seeds the v3 raw chain but hard-codes **`Enabled = false`** on every `DriverInstance`, so no children spawn from it today (its consumers inject values straight into the host); `SpawnHostAndApply` (`:118-137`) wires publish/mux/vt probes and waits for `ApplyAck(Applied)` + `RebuildAddressSpace`. +`DriverHostActor.Props` accepts `driverFactory`, `healthPublisher` and `invokerFactory` (`:518-545`) — but `PublishHealthSnapshot`'s `_healthPublisher.Publish` call is **inside** a `catch (Exception)` (`:1279-1286`), so a throwing publisher is *not* a usable crash seam for the supervision test (see resolved choice #5). +`DriverInstanceActor` handles `SetDesiredSubscriptions` in **all four** behaviours — `Stubbed` (`:389`), `Connecting` (`:428`), `Connected` (`:483`), `Reconnecting` (`:664`) — and applies a stored set on every Connected entry via `ResubscribeDesired()`/`SubscribeDesiredAlarms()` (`:416-418` initial connect, `:657-659` reconnect). So a set delivered at spawn time, while the child is still `Connecting`, is stored and applied at Connected entry: **the send is safe from `SpawnChild`.** +`DataChangeEventArgs` (`Core.Abstractions/ISubscribable.cs:50-53`) is a positional record with **no null validation**, and `DriverInstanceActor.OnDataChangeForward` dereferences `msg.Snapshot` unguarded (`:1189-1196`) on the actor thread — the crash seam the supervision test uses (resolved choice #5). +AdminUI restart surface: `/hosts` → `DriverStatusPanel.razor:332-356` (`RestartConfirmedAsync` → `AdminOps.AskAsync(new RestartDriver(...))`) → `AdminOperationsActor.HandleRestartDriverAsync` (`:430-458`) → mesh transport → node `EventStream` → `DriverHostActor.HandleRestartDriver`. Gated by the `DriverOperator` policy; the docker-dev rig runs `Security__Auth__DisableLogin=true`, so the button is drivable without a sign-in. + +### Open design choices — resolved in this plan + +1. **S16 fix shape** (report recommends "re-run `PushDesiredSubscriptions` for the instance after `SpawnChild` in the restart path"): resolved to **option (c) — `SpawnChild` always sends the desired set from a cached per-driver authority**, not a re-push bolted onto the restart handler. Rationale in Finding 1's alternatives; the short version is that the report's fix is the shape that produced the bug. +2. **`_lastComposition`**: resolved to **delete it**, not to finally read it. The payload a spawn needs is the per-driver `(valueRefs, alarmRefs)` pair, which is derived once per apply and cached (`_desiredByDriver`); keeping a whole `AddressSpaceComposition` alive so a future call site can re-derive the same thing is how the field became write-only in the first place. The deletion is the finding's actual close. +3. **Supervision restart in or out of scope**: **in.** It is the same defect one incarnation deeper, it is strictly worse (the driver object is *shut down* and never re-initialised), the fix is ~15 lines once the desired-set authority exists, and U6 explicitly asks for the first actor-restart test — writing that test without the supervision case would leave the worse half untested and unfixed for a fourth round. +4. **Bounded/backoff supervision strategy** (S6's other half — "no backoff supervision"): **explicitly OUT of scope**, deferred with a reason. Replacing Akka's default decider changes failure semantics for every driver child fleet-wide (a `Stop` directive silently removes a driver; a bounded retry count silently stops one after N crashes), and the reprovision hook below makes the *current* infinite-restart behaviour correct rather than dead. Revisit only with its own live gate. +5. **Supervision-test crash seam**: resolved to the **driver's own `OnDataChange` event carrying a null `Snapshot`** — a test-only method on the recording stub raises `DataChangeEventArgs(handle, "ref", null!)`; the actor marshals it to `Self` and `OnDataChangeForward` NREs on the actor thread ⇒ unhandled ⇒ default decider ⇒ Restart. Rejected alternatives: a throwing `IDriverHealthPublisher` (swallowed at `:1283-1286`), a throwing `SubscribeAsync` (swallowed — the stub already models it with `SubscribeFailuresBeforeSuccess`), `Kill.Instance` (the default decider **Stops** on `ActorKilledException`, which is a different path), and an `internal` "throw this" test message (a production surface added purely for a test). The fault is arbitrary by design — what is under test is the **recovery**, not the fault. If `DataChangeEventArgs` gains null validation later, the fallback is an explicit `SupervisorStrategy` on the host plus an internal test message; the task records which landed. +6. **S17 fix shape**: `max(LastSuccessfulRead, _connectedSinceUtc)`, exactly as the `:458-459` comment already promises. Rejected: (a) resetting `LastSuccessfulRead` on reconnect — that is driver-side state this actor does not own, and several drivers deliberately preserve it (the #527 design); (b) a separate "grace ticks" counter — re-introduces the tick-counting the #527 conjunction was chosen to avoid; (c) skipping the watchdog for the first N ticks after a connect — same thing with a worse name, and it silently shortens the grace whenever the poll interval is retuned. +7. **Live-gate scope**: the operator-restart leg is **merge-blocking**; the S17 leg is merge-blocking; the supervision leg is **not** live-gated (it cannot be provoked from an operator surface) and is covered by the unit test — recorded as such rather than quietly skipped. + +--- + +## Finding 1 — 03/S16: Operator "Restart driver" yields a subscription-less child until the next deploy (High) + +### Restatement + +`HandleRestartDriver` (`:2916-2932`) stops the child and respawns it from the cached spec, but the only `SetDesiredSubscriptions` sender in the repo is the apply-time bulk loop (`:2664`). The fresh child starts with `_desiredRefs = Array.Empty()`, so nothing subscribes; the #488 periodic reconcile is structurally inert on an empty set (`:625`); and the driver therefore reads nothing, publishes nothing and raises no native alarms **until the next deployment dispatch**. An operator pressing a button labelled "Restart" to recover a sick driver instead silently kills it. `_lastComposition` — declared `:349`, assigned `:2709`, read nowhere — is precisely the state a restart re-push was meant to consume, landed and never wired. + +**Extension confirmed by this plan's verification:** the same defect exists one incarnation deeper and is worse. With no `SupervisorStrategy` anywhere, a child that throws is **Restarted** by Akka's default decider; the restart runs `PostStop` (→ `IDriver.ShutdownAsync`) and then `PreStart`, but `InitializeRequested` is never re-sent (its only sender is inside `SpawnChild`, which a restart does not run) — so the child sits in `Connecting` forever around a shut-down driver, with its desired set gone too. **Force Reconnect is unaffected** (same actor instance; `ResubscribeDesired()` runs on the reconnect's Connected entry) — verified, and pinned by a new regression test so this plan's edits cannot break it. + +### Verification + +Confirmed exact (see summary table). Additionally: + +- Repo-wide, `SetDesiredSubscriptions` is sent from exactly one line in `src/`: `DriverHostActor.cs:2664`. +- `SpawnChild` has exactly two callers: `ApplyDriverSpecs` (`:2235`, the deploy reconcile) and `HandleRestartDriver` (`:2931`). The deploy path is followed by `ApplyDesiredSubscriptions`; the restart path is followed by nothing. +- The `SendAuthoredOnly` comment (`:2655-2659`) claims the helper is "shared by the bulk loop AND the dropped-driver fallback so the two CANNOT drift". There is **no dropped-driver fallback** — the helper has one call site. The comment describes an intent this plan actually restores (theme 6 doc drift: fix it in the same commit). +- `RestartDriver_respawns_the_child_without_an_actor_name_collision` passes against the broken behaviour: it asserts existence, not function. + +### Root cause + +Two spawn paths, one payload sender. The desired-subscription set is treated as a *step in the apply pipeline* rather than as *part of what it means to have a running child*, so every new way of getting a child has to remember to re-run a step that lives somewhere else. The restart path (added with #516's respawn work, whose generation-suffixed actor names it inherits) forgot; the supervision path never had the chance, because it produces a new incarnation without going through `SpawnChild` at all. + +### Proposed design + +**One desired-set authority, consumed by `SpawnChild` itself.** + +**(1) Cache the per-driver payload where the routing maps are built** (`DriverHostActor`, replacing `_lastComposition`): + +```csharp +/// One driver child's desired subscription payload — exactly the SetDesiredSubscriptions +/// the apply's bulk push sent it. +private readonly record struct DesiredSubscriptionSet( + IReadOnlyList ValueRefs, + IReadOnlyList AlarmRefs); + +/// Per-driver desired subscription sets from the most-recent apply. THE authority: the bulk +/// push, and the supervision-reprovision handler all send from here, so a +/// child obtained by ANY route carries the same set. Cleared and repopulated whole on every apply, +/// alongside the routing maps, so renames/removals can never leave a stale entry. Empty until the +/// first apply — a child spawned before any apply is handed nothing, which is what it should have. +private readonly Dictionary _desiredByDriver = new(StringComparer.Ordinal); +``` + +Populated in `ApplyDesiredSubscriptions` in the same clear-and-repopulate pass as `_nodeIdByDriverRef`/`_alarmNodeIdByDriverRef` (`:2596-2605` clear; fill after `refsByDriver` / `alarmRefsByDriver` are built at `:2554-2578`), keyed over the **union of** `refsByDriver.Keys`, `alarmRefsByDriver.Keys` and `_children.Keys` — the last one matters: a running child with no authored tags must get an explicit **empty** set, which is what `SendAuthoredOnly`'s `TryGetValue`-else-empty already produces today and what the child routes to `Unsubscribe` (`:485-486`). + +**(2) Promote the local function to the single send helper:** + +```csharp +/// Send 's desired subscription payload to . +/// The ONE place a SetDesiredSubscriptions is built, so the apply push, a respawn and a supervision +/// reprovision cannot drift. An EMPTY set is valid and meaningful — the child routes it to Unsubscribe +/// (dropping a stale handle) rather than to a spurious subscribe. +/// The value-ref count, for the bulk loop's log total. +private int SendDesiredSubscriptions(IActorRef actor, string driverId) +{ + var set = _desiredByDriver.TryGetValue(driverId, out var s) + ? s + : new DesiredSubscriptionSet(Array.Empty(), Array.Empty()); + actor.Tell(new DriverInstanceActor.SetDesiredSubscriptions( + set.ValueRefs, SubscriptionPublishingInterval, set.AlarmRefs)); + return set.ValueRefs.Count; +} +``` + +The bulk loop (`:2668-2672`) calls it unchanged in shape; the stale "dropped-driver fallback" comment is rewritten to name the three real consumers. + +**(3) `SpawnChild` sends it, for both the stub and real branches**, immediately after `_children[spec.DriverInstanceId] = new ChildEntry(...)` (`:2782`) — i.e. after the `InitializeRequested` Tell (`:2779`), which keeps init-before-subscribe ordering: + +```csharp +// EVERY spawn hands the child its desired set — deploy reconcile, operator Restart (#S16), and the +// supervision reprovision below. Sending it here rather than at each call site is the point: a child +// that exists without its subscription set is a dead driver, so the two facts are established together. +SendDesiredSubscriptions(child, spec.DriverInstanceId); +``` + +**Ordering during a deploy, stated explicitly (and pinned by a test):** `ApplyDriverSpecs` (which spawns) runs *before* `ApplyDesiredSubscriptions` (which rebuilds `_desiredByDriver`), so a **newly-spawned** child is handed the *previous* apply's set — or nothing, on the first deploy. That transient set is superseded milliseconds later by the bulk push on the same sender→receiver pair (Akka guarantees per-pair ordering), and the child is still in `Connecting`, where `SetDesiredSubscriptions` only **stores** (`:428`) and the subscribe happens at Connected entry (`:416-418`). So the stale set is never acted on. It is nonetheless documented in the xmldoc, and T5 pins "after a fresh deploy the child's live subscription carries the NEW refs". + +**(4) Supervision reprovision** — `DriverInstanceActor` gains one message and one override: + +```csharp +/// Sent to the parent from so the host re-provisions THIS +/// incarnation. A supervision restart recreates the actor object: the desired subscription sets are +/// gone, and PreRestart already ran PostStop — which called on the +/// driver object the Props closure re-uses. Nothing else re-sends InitializeRequested (its only sender +/// is DriverHostActor.SpawnChild, and a restart does not respawn), so without this the restarted child +/// sits in Connecting forever around a shut-down driver. +public sealed record ReprovisionRequested(string DriverInstanceId, string Reason); + +/// +protected override void PostRestart(Exception reason) +{ + base.PostRestart(reason); // runs PreStart: re-attach sources, restart the health timer + Context.Parent.Tell(new ReprovisionRequested(_driverInstanceId, reason.GetType().Name)); +} +``` + +`DriverHostActor` handles it in the three `Receive` blocks that already accept `RestartDriver` (`:1078`, `:1133`, `:1715`): + +```csharp +private void HandleReprovisionRequested(DriverInstanceActor.ReprovisionRequested msg) +{ + if (!_children.TryGetValue(msg.DriverInstanceId, out var entry)) return; // already stopped + // A crash that Akka silently restarted is otherwise invisible to operators; name it. + _log.Warning("DriverHost {Node}: driver child {Id} was restarted by supervision after {Reason} — re-provisioning", + _localNode, msg.DriverInstanceId, msg.Reason); + if (!entry.Stubbed) entry.Actor.Tell(new DriverInstanceActor.InitializeRequested(entry.Spec.DriverConfig)); + SendDesiredSubscriptions(entry.Actor, msg.DriverInstanceId); +} +``` + +The child's `IActorRef` survives a restart, so `_children` needs no maintenance; the dependency-consumer mux adapter is a *separate* child of the host and is unaffected by the instance actor's restart. + +**(5) Delete `_lastComposition`** (`:347-349`, `:2709`) and the stray blank line at `:2710`. Fix the two stale comments named above. + +**Alternatives considered:** + +- *(a) Re-push after `SpawnChild` inside `HandleRestartDriver` only* — the report's own recommendation. **Rejected as the primary fix**: it repairs one of three incarnation paths, leaves the strictly-worse supervision path untouched, and is structurally the same "remember to also call X" pattern that produced the bug. It is a strict subset of the chosen design, so nothing is lost by going one level up. +- *(b) Child pulls its desired set on start* (`PreStart` → `Context.Parent.Tell(new DesiredSubscriptionsRequested(id))`) — attractive because it covers restart automatically. **Rejected as the primary**: it adds a round-trip to every ordinary deploy spawn and inverts the established push direction for the common path. **But it is exactly the shape adopted for the supervision case** (item 4), where the parent has no other notification that a new incarnation exists. Hybrid: push on spawn, pull on restart. +- *(c) Have the restart re-run `ApplyDesiredSubscriptions(_lastComposition)`* — i.e. actually read the write-only field. **Rejected**: one driver's restart would re-Tell **every** child, re-Tell the VirtualTag and ScriptedAlarm hosts their whole plans, and rebuild every routing map — a node-wide side effect for a single-driver operation, and a needless re-parse of state that is trivially cached per driver. +- *(d) Keep `_lastComposition` and document it as future state* — **rejected**: a field whose only purpose is a hypothetical future reader is the finding. +- *(e) Change the supervision directive to `Stop` + host `Context.Watch`/respawn* — cleaner in theory (one path: everything goes through `SpawnChild`), **rejected here** as a fleet-wide failure-semantics change deserving its own plan and live gate (resolved choice #4); the reprovision hook makes the current default correct without changing when a driver lives or dies. + +### Implementation steps + +1. `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs` — `DesiredSubscriptionSet` + `_desiredByDriver` (replacing `_lastComposition` at `:347-349`); populate in `ApplyDesiredSubscriptions`; promote `SendAuthoredOnly` (`:2660-2666`) to `SendDesiredSubscriptions`; call it from `SpawnChild` (`:2782`); `HandleReprovisionRequested` + three `Receive` registrations (`:1078`, `:1133`, `:1715`); delete `_lastComposition` assignment (`:2709`); rewrite the `:2655-2659` comment. +2. `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs` — `ReprovisionRequested` record + `PostRestart` override. +3. `docs/DriverLifecycle.md` — new §"What a (re)spawn re-provisions": the three incarnation routes (deploy reconcile / operator Restart / supervision restart), what each re-sends, and the verified fact that Force Reconnect re-uses the live instance and needs nothing. + +### Tests + +New `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorRestartTests.cs` — **the first actor-restart test in the repo (03/U6)**. Harness: `RuntimeActorTestBase` + `V3ArtifactSeeder` with a new optional `enabled: true` (see below) so the seeded v3 deployment both **spawns children** and **carries raw tags for them**, plus a new `RecordingSubscribableDriverFactory` that keeps every driver it created (`IReadOnlyList Created`) and stamps each with the spec's `DriverInstanceId`. + +- **`Operator_restart_re_subscribes_the_respawned_child`** — *the falsification.* Deploy → `Created[0].SubscribeCount == 1` with `LastSubscribedRefs == ["Plant/Modbus/dev1/speed"]`; Tell `RestartDriver`; await `Created.Count == 2` **and** `Created[1].SubscribeCount >= 1` **and** `Created[1].LastSubscribedRefs` equals the same deployed RawPaths. **Fails on current code** — the second driver is created and connected, but never subscribed. Explicitly *not* asserted as the pass criterion: that a second driver was created, or that one child remains (that is `RestartDriver_respawns_the_child_without_an_actor_name_collision`, which passes against the bug). +- **`Operator_restart_re_subscribes_alarm_refs_too`** — same shape with an alarm-bearing raw tag: the respawned driver's `SubscribeAlarmsAsync` is called again (recording `IAlarmSource` stub). The value and alarm sets travel in one message but through two child code paths (`:483-496`), so both need pinning. +- **`A_child_spawned_by_a_fresh_deploy_ends_with_the_NEW_refs`** — the spawn-time-set-is-stale guard: apply revision A (one tag), then revision B (a *different* tag) which respawns nothing but re-pushes; and separately a deploy that spawns a brand-new driver, asserting its live subscription carries B's refs, not A's. Pins that the transient stale set from step (3) is never acted on. +- **`Restart_of_a_driver_with_no_authored_tags_sends_an_empty_set_not_nothing`** — the empty-set semantics are load-bearing (`Unsubscribe`, not "skip"); asserts the child received a `SetDesiredSubscriptions` with zero refs rather than no message. +- **`Supervision_restart_reinitialises_and_re_subscribes_the_child`** — a genuine crash: after the deploy, the recording driver raises `OnDataChange` with a **null Snapshot** (test-only `FireMalformedDataChange()`), which NREs on the actor thread and trips the default decider into `Restart`. Assert the SAME driver object is re-initialised (`InitializeCount` 1 → 2 — the Props closure re-uses it) and re-subscribed (`SubscribeCount` increments, `LastSubscribedRefs` correct). **Fails on current code** in the harsher way: no re-init at all. Record in the task which crash seam actually landed. +- **`Force_reconnect_keeps_the_desired_set_and_re_subscribes`** — the verified-negative regression: `ReconnectDriver` → the **same** driver object's `SubscribeCount` increments with the same refs, and `Created.Count` stays 1 (no respawn). Guards the "fix the restart path" edits from breaking the path that was already correct. + +Harness edits: `V3ArtifactSeeder.SeedV3Deployment` gains a trailing optional `bool enabled = false` (every existing call compiles and behaves identically) feeding the `DriverInstances[].Enabled` field; `SpawnHostAndApply` gains a trailing optional `IDriverFactory? driverFactory = null` forwarded to `DriverHostActor.Props`. + +Regression: `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~DriverHostActor"` (all 25 host suites) and `…~DriverInstanceActor` must stay green — in particular `DriverHostActorReconcileTests` (the existing restart test keeps passing; it is now the weaker half of a pair) and `DriverInstanceActorSubscriptionReconcileTests` (#488). + +### Effort + Risk + +**Effort:** M (the production change is ~60 lines; the test fixtures are the bulk). **Risk:** **medium** — this changes what every driver child receives at spawn, on the deploy path as well as the restart path. Mitigations: the payload is byte-identical to what the bulk push already sends (same helper), the extra spawn-time send is provably superseded and un-acted-on (documented + pinned), the empty-set case is explicitly preserved, and the full 25-suite host regression runs before the new tests are trusted. The supervision hook is additive (a new message on a path that currently does nothing at all). + +--- + +## Finding 2 — 03/S17: The comms-loss watchdog's post-reconnect grace collapses to one health tick (Medium) + +### Restatement + +`EvaluateCommsLossWatchdog` measures silence from `observed.LastSuccessfulRead ?? _connectedSinceUtc` (`:574`). Drivers deliberately **preserve** `LastSuccessfulRead` across `Degraded`/`Faulted` health writes — that preservation *is* the #527 design, and the comment at `:551-556` says so. So for a device that accepts a TCP connection but never serves data (a half-broken PLC, a wrong unit id, a firewalled read path), the sequence is: trip → reconnect succeeds → `_connectedSinceUtc` is re-stamped → the very first health tick of the new connection sees non-Healthy **and** a read stamp frozen from before the outage (already far older than 90 s) → **re-trip**. The result is reconnect churn at health-tick cadence (~30 s in production) instead of the designed 90 s grace *per connection*. The `??` only ever helps a driver that has never read at all; the comment at `:458-459` describes `max(...)`, which is what was meant. + +### Verification + +Confirmed exact at `:574` (see summary). The `_connectedSinceUtc` re-stamp on every Connected entry (`:460`) is present and correct — it is only the *consumption* that is wrong. The watchdog runs only from the Connected behaviour's health tick, so `_connectedSinceUtc` is always a real timestamp by the time it is read (never `default`). Re-verified that nothing else reads `_connectedSinceUtc`. + +The existing suite already contains the falsification control for the **other** term of the max: `A_degraded_driver_that_is_still_reading_successfully_is_left_alone` holds a driver whose `LastSuccessfulRead` advances on every call while its connect ages past the threshold (it asserts `ObservedSpan > FastTimeout` for exactly this reason), so an implementation that measured from `_connectedSinceUtc` **only** turns that test red. + +### Root cause + +`??` is "use B when A is absent"; the requirement is "use whichever is later". The two coincide only for a driver that has never read. The nullable-coalescing spelling reads plausibly enough that it survived review, and no test exercised a *second* connection — the suite's reconnect test (`The_driver_recovers_on_its_own_once_the_device_returns`) makes the device healthy again on reconnect, so the frozen-stamp case never arises. + +### Proposed design + +One line, plus the comment that explains why it is not `??`: + +```csharp +// Measure silence from the LATER of the last successful read and THIS connect — the grace is per +// connection. `??` was wrong: drivers PRESERVE LastSuccessfulRead across Degraded/Faulted writes (that +// preservation is the #527 design), so after a trip + reconnect to a device that accepts the connection +// but still serves no data, the stamp is already older than the threshold and the FIRST tick of the new +// connection re-trips — churn at health-tick cadence instead of the designed grace. `??` only ever helped +// a driver that had never read at all. +var lastEvidence = observed.LastSuccessfulRead is { } read && read > _connectedSinceUtc + ? read + : _connectedSinceUtc; +``` + +**What does NOT change:** the conjunction (state ∧ frozen-evidence) is untouched; the disable-at-zero hatch is untouched; a driver that genuinely stops reading *within* a stable connection still trips after the threshold measured from its last read, exactly as today; a half-broken device still ends up in a reconnect cycle — the fix **paces** that cycle at one attempt per grace window instead of one per health tick. That last point is deliberate and must be stated in the docs, because "the churn stopped entirely" is the wrong expectation. + +**Alternatives considered:** see resolved choice #6 — (a) reset `LastSuccessfulRead` on reconnect (driver-owned state this actor must not rewrite, and the preservation is load-bearing elsewhere); (b) a post-connect grace-tick counter (re-introduces tick counting, and silently re-scales when `HealthPollInterval` is retuned); (c) skip the watchdog for the first N ticks (same, worse). + +### Implementation steps + +1. `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs:574` — the max, with the comment above; extend the `EvaluateCommsLossWatchdog` remarks with a short "the grace is per connection" paragraph. +2. `docs/DriverLifecycle.md` §"The comms-loss watchdog" — one paragraph: the grace is per connection, a half-broken device cycles at ≤ one attempt per `DefaultCommsLossTimeout`, and the watchdog is not disabled by the change. + +### Tests + +All in the existing `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorCommsLossWatchdogTests.cs`, reusing `HealthControlledStubDriver` unchanged (the stable-health stub — `StubDriver`'s `DateTime.UtcNow` health makes any watchdog test built on it vacuous). Timings for this pair: `commsLossTimeout: 1500 ms`, `healthPollInterval: 50 ms` (the class's `FastPoll`), `reconnectInterval: 100 ms` — a grace an order of magnitude above the poll, so the absence window below is not a race. + +**The pair (each term of the max has a test that goes red when the other is deleted):** + +1. **`A_reconnected_driver_gets_a_full_grace_window_before_the_watchdog_trips_again`** *(NEW — red on current code)*. Driver: permanently `Degraded` with `LastSuccessfulRead` frozen at `UtcNow.AddMinutes(-5)`. Connect ⇒ first trip (`ConnectivityChanged(false)`). Reconnect succeeds ⇒ `ConnectivityChanged(true)` — capture the instant. Then `ExpectNoMsg(400 ms)` with a **positive control**: assert `HealthReadCount` advanced by ≥ 4 inside that window (so "no trip" cannot pass because nothing ran) and that the window is well short of the 1500 ms grace. On current code the re-trip lands within ~50-100 ms of the reconnect ⇒ **FAIL**. This is the test that goes red if the `_connectedSinceUtc` term is removed (i.e. if `??` is restored). +2. **`A_degraded_driver_that_is_still_reading_successfully_is_left_alone`** *(EXISTING — the other control)*. Its driver's `LastSuccessfulRead` advances on every call while its connect ages past the threshold, and it already asserts `ObservedSpan > FastTimeout`. It goes red if the `LastSuccessfulRead` term is removed (a `_connectedSinceUtc`-only measure trips it). Add a one-line cross-reference in both tests' xmldoc naming the other as its falsification partner, per the house convention. +3. **`A_half_broken_device_is_still_declared_lost_once_the_grace_elapses`** *(NEW — anti-vacuity)*. Same driver as (1); after the grace window, `ExpectMsg(Connected: false)` within a generous presence budget. Without this, a "fix" that simply disabled the watchdog after the first connection would pass (1) and look correct. + +**Regression to re-run and eyeball, not just to keep green:** `A_reconnect_attempt_tears_the_dead_transport_down_before_re_initialising` deliberately depends on the *repeated* trip cycle ("this driver reports stale health even after a successful re-init, so the watchdog trips again and the cycle repeats — the real behaviour for a device that accepts a connection but serves no data"). Under the fix each cycle costs a full `FastTimeout` (250 ms) instead of one 50 ms tick; its `AwaitCondition(InitializeCount >= 2, 5 s)` still has ample headroom, and the invariant it asserts (every `init` preceded by a `shutdown`) is unchanged. If it ever gets tight, raise the AwaitCondition budget — never shorten the grace. + +### Effort + Risk + +**Effort:** S (one line + three tests). **Risk:** **low** — strictly reduces how often the watchdog fires, in exactly the case the existing comment already describes as intended; no state, no config, no message-flow change. The one behaviour to watch is that a device which dies *while connected* still trips on schedule (unchanged: its `LastSuccessfulRead` is later than its connect, so the max picks the read, as today). + +--- + +## Finding 3 — 03/U6 (carried): No actor supervision/restart test exists anywhere (Medium) + +### Restatement + +U6 is the test-coverage finding, reshaped this round to two remaining gaps; the first is "still no actor supervision/restart test anywhere (S6/S16 would both have been caught)". The one restart-adjacent test in the repo asserts only that a child *exists* after a restart, which is exactly the assertion that passes against a dead driver. This plan closes the gap for the driver-child pair, which is where the finding points; the other half of U6 (the `DisableLogin` masking problem) belongs to R3-01 (03/S14) and is not in scope here. + +### Verification + +Confirmed: grep across all 57 test projects finds no test that provokes a supervision restart of any actor; `RestartDriver` appears in exactly two tests (`DriverHostActorReconcileTests:186` and `NodeCommunicationActorTests:51`, the latter a transport-delivery test that never touches a driver child). + +### Root cause + +Restart is a *lifecycle* property, and the suite's actor tests are message-in/message-out. Nothing in the harness makes "provoke a real crash and assert what survives" convenient, so the one restart test that exists was written around the crash that was actually being fixed (the actor-name collision) rather than around what a restart is *for*. + +### Proposed design + +The tests specified in Finding 1 **are** the U6 deliverable — deliberately, rather than a separate abstract "supervision test", because a lifecycle test with nothing functional to assert degenerates into the existence check that already failed to catch this. Two things are added to make the class repeatable rather than one-off: + +- **The crash seam is documented in the test file's remarks**, including *why* the obvious seams do not work (the health publisher, subscribe failures and `Kill` are all swallowed or route elsewhere) — so the next actor that needs a restart test does not re-derive it. +- **The falsification bar is stated in the file's remarks**: an actor-restart test must assert a *function* the incarnation is supposed to perform (it subscribed / it re-initialised), never that the incarnation exists. The existing `RestartDriver_respawns_the_child_without_an_actor_name_collision` is cited by name as the example of the weaker form and kept, because its own subject (the name collision) is real. + +### Implementation steps + +None beyond Finding 1's test tasks (T5, T7) and the harness edits in T4. + +### Tests + +See Finding 1. The U6-specific acceptance is: at least one test in the repo drives a **real** Akka supervision restart (not a stop, not a stub) and asserts post-restart behaviour, and at least one asserts post-**operator**-restart behaviour, and both fail against `7c77a8c7`. + +### Effort + Risk + +**Effort:** S (folded into Finding 1). **Risk:** low — test-only. The one hazard is a flaky crash seam; the task requires the supervision test to be run 20× locally before it is trusted (`--filter` loop), matching the #500 discipline for this assembly. + +--- + +## Live gate — docker-dev (merge-blocking for S16 + S17) + +The rig runs the AdminUI with `Security__Auth__DisableLogin=true`, so **drive it yourself; do not defer to the user for sign-in.** `:9200` round-robins central-1/central-2 — rebuild **both** and confirm image parity per container before trusting any observation. + +```bash +# 1. Rebuild both centrals from the branch and bring the rig up. +docker compose -f docker-dev/docker-compose.yml up -d --build central-1 central-2 +docker inspect --format '{{.Config.Image}} {{.Image}}' otopcua-dev-central-1-1 otopcua-dev-central-2-1 # must match + +# 2. Modbus fixture (on the shared docker host, not locally). +ssh dohertj2@10.100.0.35 'cd /opt/otopcua-modbus && docker compose --profile standard up -d' +``` + +3. **Author + deploy** (AdminUI `http://localhost:9200`) a Modbus driver against `10.100.0.35:5020` in `/raw` (Folder → Driver → Device → Tag), browse-commit one holding-register tag, deploy fleet-wide. If a node times out, the hatch is `ClusterNode.MaintenanceMode`, **not** `Enabled = 0`. +4. **Establish the baseline stream** — resolve the raw NodeId, then subscribe: + +```bash +dotnet run --project src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI -- browse -u opc.tcp://localhost:4840 -r -d 4 +dotnet run --project src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI -- subscribe -u opc.tcp://localhost:4840 \ + -n "ns=2;s=Plant/Modbus/dev1/Speed" -i 500 +``` + +Values must be printing before the restart. Leave this running for the whole gate. + +5. **S16 leg.** AdminUI `/hosts` → expand the Modbus driver's status panel → **Restart** → confirm. + - **Before the fix (run this on a `master` build first — the gate is only meaningful with both halves):** the CLI subscription goes silent immediately and **never recovers**; `docker logs otopcua-dev-central-1-1 | grep -E "restarting driver|SubscribeBulk"` shows the restart line with **no** subscription push after it; the driver's panel shows Connected with zero subscribed refs. + - **After the fix:** within one connect cycle the CLI resumes printing values with fresh timestamps, and the log shows the spawn-time push immediately after `restarting driver`. **Assert values RESUME, not merely that the driver reads Connected** — Connected is what it already said while dead. + - Repeat once with an **alarm-bearing** raw tag and confirm a transition still reaches `/alerts` after the restart. +6. **S17 leg.** Point a second Modbus driver at a device that accepts TCP but fails reads (the fixture's `exception_injector`, or the fixture host/port with an unmapped unit id), then `docker logs -f otopcua-dev-central-1-1 | grep "declaring comms lost"`. + - **Before:** the line repeats at health-tick cadence (~30 s). + - **After:** at most one per `DefaultCommsLossTimeout` (90 s) per connection. + - **Positive control (required):** stop the Modbus fixture entirely and confirm the driver still declares comms lost within ~90 s. Without it, "the churn stopped" is indistinguishable from "the watchdog is disabled". +7. **Supervision leg:** not provokable from an operator surface — **not live-gated by design**; covered by `Supervision_restart_reinitialises_and_re_subscribes_the_child`. Record that explicitly rather than leaving it looking untested. +8. Record all outcomes (including the before-fix observations) in `archreview/plans/STATUS.md`. + +--- + +## Task breakdown + +Branch: one branch `fix/archreview-r3-03-restart-resubscribe-watchdog` off master. **This plan lands before R3-02** (same file, different methods). Ordering: T1 (S17) is independent of everything and can land on its own; T2 → T3 → T6 is the production spine; T4 → T5/T7 are the tests; T9 last. + +Verification bar: every task has a deterministic unit guard, and the two headline tests (T5, T7) must be **seen failing against unmodified `7c77a8c7` behaviour** before the fix — an actor-restart test that only proves the child exists is the exact vacuity U6 names. Follow TDD: write the failing test, see it FAIL, implement, see it PASS. + +--- + +### T1 — S17: watchdog grace is per connection (`??` → max) + the falsification pair + +**Classification:** standard (one-line behaviour narrowing, no state/config/message change) +**Estimated implement time:** 5 min +**Parallelizable with:** everything (different file from T2–T7's production edits; different test file) +**Files:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs`, `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorCommsLossWatchdogTests.cs` + +1. Write `A_reconnected_driver_gets_a_full_grace_window_before_the_watchdog_trips_again` (frozen `LastSuccessfulRead`, `commsLossTimeout: 1500 ms`, `reconnectInterval: 100 ms`; after the second `Connected:true`, `ExpectNoMsg(400 ms)` with a `HealthReadCount` +≥4 positive control) and `A_half_broken_device_is_still_declared_lost_once_the_grace_elapses`. +2. `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~DriverInstanceActorCommsLossWatchdogTests"` → the new grace test **FAILS** (re-trip inside the absence window), the still-trips test passes. +3. Implement the max at `:574` with the comment from Finding 2; extend the method remarks with "the grace is per connection". +4. Add the cross-reference xmldoc lines making the new test and the existing `A_degraded_driver_that_is_still_reading_successfully_is_left_alone` each other's named falsification partner. +5. Re-run the filter → **PASS (all 8+ cases)**; confirm `A_reconnect_attempt_tears_the_dead_transport_down_before_re_initialising` still passes and note its now-slower cycle. +6. Commit: `fix(runtime): comms-loss grace is per connection — max(LastSuccessfulRead, connectedSince) (archreview 03/S17)`. + +### T2 — Desired-subscription payload authority (`_desiredByDriver` + `SendDesiredSubscriptions`) + +**Classification:** medium-risk (refactor of the deploy-path push; no behaviour change intended) +**Estimated implement time:** 5 min +**Parallelizable with:** T1, T4, T8; NOT T3/T6 (same file) +**Files:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs` + +1. Add `DesiredSubscriptionSet` + `_desiredByDriver`; populate it in `ApplyDesiredSubscriptions` over the union of `refsByDriver.Keys ∪ alarmRefsByDriver.Keys ∪ _children.Keys`, cleared with the routing maps. +2. Promote the `SendAuthoredOnly` local function (`:2660-2666`) to the private method `SendDesiredSubscriptions(IActorRef, string)` reading `_desiredByDriver`; bulk loop unchanged in shape. Rewrite the stale `:2655-2659` "dropped-driver fallback" comment to name the real consumers. +3. **Behaviour-preservation gate:** `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~DriverHostActor"` → **PASS unchanged** (25 suites). This task must move no test. +4. Commit: `refactor(runtime): one desired-subscription payload authority (_desiredByDriver)`. + +### T3 — `SpawnChild` always sends the desired set; delete the write-only `_lastComposition` + +**Classification:** HIGH-RISK (every driver child's spawn-time state changes, deploy path included) +**Estimated implement time:** 5 min +**Parallelizable with:** T1, T4, T8; after T2 +**Files:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs` + +1. Call `SendDesiredSubscriptions(child, spec.DriverInstanceId)` at the end of `SpawnChild` (both stub and real branches), with the ordering xmldoc from Finding 1 step (3). +2. Delete `_lastComposition` (`:347-349`), its assignment (`:2709`) and the stray blank line (`:2710`). +3. Regression gate BEFORE the new tests: `--filter "FullyQualifiedName~DriverHostActor"` → **PASS unchanged**; also `…~DriverInstanceActor` (the child now receives one extra message while `Connecting`, which `:428` stores). +4. Commit: `fix(runtime): every spawn hands the child its desired subscriptions (archreview 03/S16)`. + +### T4 — Test harness: seeder `enabled` flag + recording subscribable driver factory + +**Classification:** standard (test-only, additive + source-compatible) +**Estimated implement time:** 5 min +**Parallelizable with:** T1–T3, T6, T8 +**Files:** `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Harness/V3ArtifactSeeder.cs`, `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/StubDrivers.cs` + +1. `SeedV3Deployment` gains a trailing `bool enabled = false` feeding `DriverInstances[].Enabled`; `SpawnHostAndApply` gains a trailing `IDriverFactory? driverFactory = null` forwarded to `DriverHostActor.Props`. Both defaults preserve every existing call site verbatim. +2. Add `RecordingSubscribableDriver` (per-instance `DriverInstanceId`, `ISubscribable` + optional `IAlarmSource`, counting `InitializeCount`/`ShutdownCount`/`SubscribeCount`/`SubscribeAlarmsCount`, recording `LastSubscribedRefs`/`LastSubscribedAlarmRefs`, plus `FireMalformedDataChange()` raising `OnDataChange` with a null `Snapshot` for the crash seam) and `RecordingSubscribableDriverFactory` exposing `IReadOnlyList Created`. +3. Build + `--filter "FullyQualifiedName~V3"` and the existing fan-out suites → **PASS unchanged**. +4. Commit: `test(runtime): recording subscribable driver factory + enabled-driver v3 seed`. + +### T5 — `DriverHostActorRestartTests` — the operator-restart falsification (03/U6's first half) + +**Classification:** high-risk verification (guards T3; the falsification bar) +**Estimated implement time:** 5 min per case +**Parallelizable with:** T1, T6, T8; after T3 + T4 +**Files:** `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorRestartTests.cs` (new) + +1. Write `Operator_restart_re_subscribes_the_respawned_child` and run it **against a build with T3 reverted** (or before T3 lands): it must **FAIL** with `Created[1].SubscribeCount == 0`. Record that observation in the commit message — a restart test that has never been seen red is the finding. +2. Add `Operator_restart_re_subscribes_alarm_refs_too`, `A_child_spawned_by_a_fresh_deploy_ends_with_the_NEW_refs`, `Restart_of_a_driver_with_no_authored_tags_sends_an_empty_set_not_nothing`. +3. File-level remarks: the falsification bar (assert a *function*, never existence), naming `RestartDriver_respawns_the_child_without_an_actor_name_collision` as the weaker form that is kept for its own subject. +4. `--filter "FullyQualifiedName~DriverHostActorRestartTests"` → **PASS**; run 20× for flake. +5. Commit: `test(runtime): operator restart re-subscribes the respawned child (archreview 03/S16, U6)`. + +### T6 — Supervision reprovision hook (`ReprovisionRequested` + `PostRestart`) + +**Classification:** medium-risk (new message on a path that currently does nothing; no existing behaviour replaced) +**Estimated implement time:** 5 min +**Parallelizable with:** T1, T4, T8; after T3 +**Files:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs`, `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs` + +1. `ReprovisionRequested` record + `PostRestart` override (calling `base.PostRestart(reason)` first — it runs `PreStart`, which re-attaches the rediscovery/host-status sources and the health timer). +2. `HandleReprovisionRequested` on the host (Warning log naming the crash type — a supervision restart is currently **completely silent** to operators — then `InitializeRequested` for a non-stub child, then `SendDesiredSubscriptions`), registered in the three `Receive` blocks at `:1078`, `:1133`, `:1715`. +3. `--filter "FullyQualifiedName~DriverHostActor"` and `…~DriverInstanceActor` → **PASS unchanged**. +4. Commit: `fix(runtime): re-provision a driver child after a supervision restart (archreview 03/S6, S16)`. + +### T7 — Supervision-restart + force-reconnect tests (03/U6's second half) + +**Classification:** high-risk verification (guards T6; the first real supervision-restart test in the repo) +**Estimated implement time:** ~15 min (the crash seam is the one uncertainty) — the deliberately-larger task +**Parallelizable with:** T1, T8; after T4 + T6 +**Files:** `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorRestartTests.cs` + +1. `Supervision_restart_reinitialises_and_re_subscribes_the_child`: deploy, drive the child Connected, `FireMalformedDataChange()`, then assert the **same** driver object's `InitializeCount` 1 → 2 and `SubscribeCount` increments with the deployed refs. Confirm first that it FAILS with T6 reverted (no re-init at all). +2. **If the null-`Snapshot` seam does not produce a `Restart`** (e.g. the record gains validation), fall back to the named alternative: an explicit `OneForOneStrategy(Directive.Restart)` on the host plus an `internal` test-only throw message under `InternalsVisibleTo`. **Record in the commit message which seam landed** and why. +3. `Force_reconnect_keeps_the_desired_set_and_re_subscribes` — the verified-negative regression (same driver object re-subscribes; `Created.Count` stays 1). +4. Run the class 20× (`--filter "FullyQualifiedName~DriverHostActorRestartTests"`) for flake; this assembly runs ~14-way parallel (#500). +5. Commit: `test(runtime): first supervision-restart test — child re-initialises + re-subscribes (archreview 03/U6)`. + +### T8 — Docs + +**Classification:** standard (docs) +**Estimated implement time:** 5 min +**Parallelizable with:** all +**Files:** `docs/DriverLifecycle.md` + +New §"What a (re)spawn re-provisions": the three incarnation routes (deploy reconcile / operator Restart / supervision restart), what each re-sends, `_desiredByDriver` as the single authority, and the verified fact that **Force Reconnect re-uses the live instance and needs nothing**. Extend §"The comms-loss watchdog" with the per-connection grace, the paced (not eliminated) reconnect cycle for a half-broken device, and the note that the watchdog is not disabled by the change. Commit: `docs: restart re-provisioning + per-connection watchdog grace (R3-03)`. + +### T9 — LIVE GATE: docker-dev restart + watchdog verification — operator/rig task + +**Classification:** HIGH-RISK verification gate (merge-blocking for the S16 and S17 slices) +**Estimated implement time:** ~45 min rig session (both halves: a `master` build for the before-observation, then the branch) +**Parallelizable with:** nothing (needs the built branch) +**Files:** none (rig session; outcomes recorded in `archreview/plans/STATUS.md`) + +Run the eight-step recipe in the Live gate section: both centrals rebuilt with image-parity check; Modbus fixture up; a `/raw`-authored driver deployed; a Client.CLI subscription streaming; **before-fix observation on a `master` build** (subscription silent forever after Restart), then the branch (values resume); the alarm-tag repeat; the S17 churn-cadence comparison **with its stop-the-fixture positive control**; and an explicit record that the supervision leg is unit-tested rather than rig-tested. Record every observation — including the before-fix ones — in STATUS.md. + +--- + +### Sequencing summary + +``` +T1 (03/S17 — independent, land any time; smallest) +T2 → T3 → T6 (03/S16 production spine, sequential — same file) +T4 → T5 (operator-restart falsification; T5 after T3) +T4 → T7 (supervision restart + force-reconnect; T7 after T6) +T8 anytime; T9 last (after T3/T5/T6/T7 merge-ready) +``` + +Overall effort: **~0.75–1 dev-day code+unit, plus the rig session.** The one HIGH-RISK production slice (T3, which changes what every child receives at spawn) lands behind the full 25-suite `DriverHostActor` regression before any new test stacks on it, and behind T2's explicitly behaviour-preserving refactor. 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 new file mode 100644 index 00000000..b14841de --- /dev/null +++ b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json @@ -0,0 +1,75 @@ +{ + "planPath": "archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md", + "lastUpdated": "2026-08-20", + "tasks": [ + { + "id": "T1", + "subject": "Comms-loss grace is per connection: ?? -> max(LastSuccessfulRead, _connectedSinceUtc) + falsification pair (03/S17)", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T2", + "subject": "Desired-subscription payload authority: _desiredByDriver + SendDesiredSubscriptions (behaviour-preserving refactor)", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T3", + "subject": "SpawnChild always sends the desired set; delete the write-only _lastComposition (03/S16)", + "status": "pending", + "blockedBy": [ + "T2" + ] + }, + { + "id": "T4", + "subject": "Test harness: V3ArtifactSeeder enabled flag + RecordingSubscribableDriver(Factory) with a crash seam", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T5", + "subject": "DriverHostActorRestartTests: operator restart re-subscribes the respawned child (falsifiable; 03/S16, U6)", + "status": "pending", + "blockedBy": [ + "T3", + "T4" + ] + }, + { + "id": "T6", + "subject": "Supervision reprovision hook: DriverInstanceActor.PostRestart -> ReprovisionRequested -> host re-init + re-subscribe (03/S6, S16)", + "status": "pending", + "blockedBy": [ + "T3" + ] + }, + { + "id": "T7", + "subject": "First real supervision-restart test + force-reconnect verified-negative regression (03/U6)", + "status": "pending", + "blockedBy": [ + "T4", + "T6" + ] + }, + { + "id": "T8", + "subject": "docs/DriverLifecycle.md: what a (re)spawn re-provisions + per-connection watchdog grace", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T9", + "subject": "LIVE GATE: docker-dev restart re-subscribe (before/after) + watchdog churn cadence with stop-the-fixture control", + "status": "pending", + "blockedBy": [ + "T3", + "T5", + "T6", + "T7" + ] + } + ] +}