docs(archreview): R3-03 review outcome — F1/F2/F3 fixed with red-before evidence
This commit is contained in:
Joseph Doherty
@@ -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`.
|
||||
|
||||
Reference in New Issue
Block a user