From 6d0002889a7ef214c882b6ae4488d3f793d5410b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:39:47 -0400 Subject: [PATCH] =?UTF-8?q?docs(archreview):=20R3-03=20review=20outcome=20?= =?UTF-8?q?=E2=80=94=20F1/F2/F3=20fixed=20with=20red-before=20evidence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...R3-03-restart-resubscribe-watchdog-plan.md | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md index c1ac048d..10648423 100644 --- a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md +++ b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md @@ -685,3 +685,88 @@ After restoring the hook: **6/6 pass**, and the class was run **20× in isolatio `SpawnChild` gives the child the production 10 s `DefaultReconnectInterval` (T7). 7. `_lastComposition` and `_desiredByDriver` coexist for one commit (T2) so T2 stays compilable; the deletion lands in T3, as T3's own step text specifies. + +--- + +## Review outcome + +Verdict **FIX-FIRST**, 3 items. All three confirmed against the code, fixed TDD (test first, seen red +for the predicted reason, then fixed), one commit each. + +### F1 [HIGH regression] — a changed alarm set latched (`edd01fcc`) + +**Confirmed.** `SubscribeDesiredAlarms` and `HandleSubscribeAlarmsAsync` both gated on +`_alarmSubscriptionHandle is not null`, which is the right guard for an idempotent re-push and the wrong +one for a deploy that changes the set. The value side converges by construction +(`StoreDesiredSubscriptions` is a full replace; `HandleSubscribeAsync` drops the prior subscription +first) — the alarm side did not. **T3 is what made it reachable**: a tag-set edit forces a respawn, and +a fast-initialising driver subscribes the transient spawn-time set before the bulk push, so the +PREVIOUS revision's alarm refs latched for the connection's life. `sourceNodeIds`-filtering drivers +(OPC UA Client, AB CIP, FOCAS) would silently never deliver the newly-added alarm. + +**RED:** `A_deploy_that_widens_the_alarm_set_re_subscribes_the_new_refs` — after deploying +alarm `{alarmX}` then `{alarmX, alarmY}`, `LastSubscribedAlarmRefs` was still `["Plant/Modbus/dev1/alarmX"]`. + +**Fix:** new `_subscribedAlarmRefs` records what the live handle was established with; +`AlarmSubscriptionIsStale()` (no handle **or** the desired refs differ) replaces the bare null check in +both places; a stale handle is **released before** re-subscribing so a filtering driver cannot +accumulate two overlapping subscriptions. The release clears the handle *first*, so a throwing +unsubscribe cannot leave the staleness check treating a dead handle as live, and it is best-effort +(logged, non-fatal — the re-subscribe is what matters). Unsubscribe-on-**empty** is still deliberately +absent, unchanged. Two notes: the release had to be wrapped in `_invoker.ExecuteAsync` or the repo's +`OTOPCUA0001` analyzer fails the build; and the Connected-handler comment that stated the +value-only convergence claim was corrected, as was the matching bullet in `docs/DriverLifecycle.md`. + +### F2 [MEDIUM] — reprovision routed by id, not incarnation (`2b7100cf`) + +**Confirmed.** Fixed with `if (!Equals(Sender, entry.Actor)) return;` (plus a Debug line naming the +superseded ref). + +⚠️ **The first version of the test PASSED against the unfixed code — the arrangement mattered.** With +the child already **Connected**, `InitializeRequested` is unhandled, so the stale reprovision was +swallowed for the wrong reason and the test was vacuous. The defect only bites a child in +`Connecting`, which is exactly where `InitializeRequested` *is* handled. `RecordingSubscribableDriver` +therefore gained an `InitializeGate` (a `TaskCompletionSource` its `InitializeAsync` awaits) so the +test can hold the child in `Connecting`. **RED with that arrangement:** `InitializeCount == 2` — the +second concurrent `InitializeAsync` racing the #527 dispose-before-overwrite. The absence assertion +carries a positive control (the child is proven alive at `InitializeCount == 1` first) and a +`GetDiagnostics` round-trip so the dropped message is provably processed before the absence is read. + +### F3 [IMPORTANT] — no backoff on the reprovision (`0793b0cb`) + +**Confirmed.** Added `DefaultReprovisionBackoff` (10 s, matched to +`DriverInstanceActor.DefaultReconnectInterval` because the reprovision drives the same connect work), +a per-driver `_lastReprovisionUtc` stamp, and a `reprovisionBackoff` Props parameter following the +existing `applyWedgeTimeout` precedent so a test need not wait the production window. + +A held request is **deferred, not dropped**: one `StartSingleTimer` per driver re-delivers a +`ReprovisionDeferred` carrying the **original incarnation ref**, so F2's identity check still applies +on re-delivery, and a burst collapses to a single pending reprovision rather than a queue. Each +deferral logs a Warning naming the crash type, so a crash loop is visible rather than quiet. + +**RED** (backoff disabled): `InitializeCount == 3` where the test requires 2-then-3 — i.e. the second +reprovision applied immediately. The test drives **two genuine supervision restarts** (not synthetic +messages), waits for the re-subscribe before arming the second crash, and asserts `ShutdownCount >= 2` +so "the second crash tripped supervision" is proven before the backoff is read. + +### Optional adds (both done) + +- `A_running_driver_with_no_authored_tags_is_handed_an_empty_set` — the `_children.Keys` union arm, + previously untested and called out as a gap in the T5 notes. It needed a driver with no tags at all, + which the seeder could not express, so `SeedV3Deployment` gained `taglessDrivers`. The tagless child + is asserted initialised-but-never-subscribed, with its tag-owning sibling's subscribe as the positive + control that the push pass ran. +- `_desiredByDriver` xmldoc now records that it is rebuilt only by a **successful** apply, so it + lingers on the last-known-good composition after a failed one — consistent with the address space, + drivers and subscriptions that also keep serving (#485/#486). + +### Verification after the three fixes + +- `dotnet build ZB.MOM.WW.OtOpcUa.slnx` → **0 errors**. +- **Full `Runtime.Tests`: 649 passed, 3 skipped, 0 failed** (up 4 from 645 — F1, F2, F3 and the + union-arm test). +- **`ControlPlane.Tests`: 262/262.** +- `DriverHostActorRestartTests` (now 10 cases) run **10× in isolation, 10/10 green**. +- T9 (live gate) still **pending** — the orchestrator runs it. Worth noting for that session: F1 is + directly observable on the rig, by deploying a second alarm-bearing raw tag to a driver that already + has one and confirming a transition on the NEW tag reaches `/alerts`.