feat(drivers): consume IRediscoverable as an operator re-browse prompt (§8.2, #518)
Nine drivers raise IRediscoverable.OnRediscoveryNeeded when they observe that a remote's tag set may have changed — a Galaxy redeploy, a TwinCAT symbol-version bump, an MTConnect agent restart, a Sparkplug rebirth. Nothing in src/ subscribed, so every raise went into the void. Drivers even wrap the invoke in try/catch for "subscriber threw"; there has never been a subscriber. DriverInstanceActor now attaches the event in PreStart and detaches in PostStop, mirroring AttachAlarmSource/DetachAlarmSource. Attach is per-actor, not per-connect: the raise has no connection affinity and can land while the driver is between connects. The detach is load-bearing rather than tidy — the IDriver instance outlives the actor when the host respawns a child around the same driver object, so a missing -= accumulates one handler per respawn, each holding a dead Self. The signal rides the existing driver-health snapshot to /hosts as a "re-browse" chip. It is ADVISORY: the served address space is deliberately not rebuilt, because v3 authors raw tags explicitly through the /raw browse-commit flow and a runtime graft would materialise nodes no operator approved and no deployment artifact records. Two things worth knowing: - PublishHealthSnapshot dedups on a health fingerprint, and a rediscovery raise on an otherwise-healthy driver changes none of the four fields it hashed. Without adding the timestamp to that tuple the dedup swallows the exact publish carrying the signal. Reverting that one line turns 3 of the 5 new tests red, which is how it was confirmed rather than assumed. - The new test stub implements IDriver from scratch instead of deriving from the shared StubDriver, whose GetHealth() returns DateTime.UtcNow — its fingerprint differs on every call, so the dedup never engages and the dedup test would have passed vacuously either way. The planned discovery-result drift detector was NOT built. Modbus, S7, MQTT, AbLegacy and Sql all echo authored config from DiscoverAsync rather than browsing the device, so a discovered-vs-authored diff is permanently empty for them — it would have been another plausible-looking inert seam, which is the class this audit exists to remove. IRediscoverable is the trustworthy signal because the driver asserts it deliberately. DriverHealthChanged, IDriverHealthPublisher.Publish and HostsDriverRow gain defaulted optional parameters, so no existing call site changed. telemetry.proto gains fields 8/9 and both Phase-5 mappers carry them. Runtime.Tests 512 passed / 31 skipped.
This commit is contained in:
Joseph Doherty
+249
@@ -0,0 +1,249 @@
|
||||
using Akka.Actor;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
using ZB.MOM.WW.OtOpcUa.Runtime.Drivers;
|
||||
using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers;
|
||||
|
||||
/// <summary>
|
||||
/// Covers the <see cref="IRediscoverable"/> consumer (Gitea #518). Nine drivers raise
|
||||
/// <see cref="IRediscoverable.OnRediscoveryNeeded"/> when they observe that a remote's tag set may have
|
||||
/// changed — a Galaxy redeploy, a TwinCAT symbol-version bump, an MTConnect agent restart, a Sparkplug
|
||||
/// rebirth. Before this wiring NOTHING in <c>src/</c> subscribed, so every one of those raises went into
|
||||
/// the void.
|
||||
/// <para><b>The signal is advisory.</b> It does not rebuild the served address space: v3 authors raw tags
|
||||
/// explicitly through the <c>/raw</c> browse-commit flow, so a runtime graft would create nodes no
|
||||
/// operator approved. It rides the driver-health snapshot to the AdminUI as a re-browse prompt.</para>
|
||||
/// </summary>
|
||||
[Trait("Category", "Unit")]
|
||||
public sealed class DriverInstanceActorRediscoverySignalTests : RuntimeActorTestBase
|
||||
{
|
||||
/// <summary>
|
||||
/// The load-bearing case: a driver raises rediscovery while otherwise perfectly healthy, and the
|
||||
/// signal reaches the health publisher carrying the driver's reason.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Rediscovery_raise_reaches_the_health_publisher_with_its_reason()
|
||||
{
|
||||
var driver = new RediscoverableStubDriver();
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var parent = CreateTestProbe();
|
||||
parent.IgnoreMessages(_ => true);
|
||||
var actor = parent.ChildActorOf(DriverInstanceActor.Props(driver, healthPublisher: publisher));
|
||||
|
||||
actor.Tell(new DriverInstanceActor.InitializeRequested("{}"));
|
||||
AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(0), TimeSpan.FromSeconds(3));
|
||||
|
||||
driver.RaiseRediscovery("deploy-time-changed");
|
||||
|
||||
AwaitAssert(
|
||||
() =>
|
||||
{
|
||||
var withSignal = publisher.Published.LastOrDefault(p => p.RediscoveryNeededUtc is not null);
|
||||
withSignal.ShouldNotBeNull();
|
||||
withSignal!.RediscoveryReason.ShouldBe("deploy-time-changed");
|
||||
},
|
||||
TimeSpan.FromSeconds(3));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <b>Regression guard for the dedup trap.</b> <c>PublishHealthSnapshot</c> suppresses a publish whose
|
||||
/// (state, lastSuccessfulRead, lastError, errorCount) tuple repeats. A rediscovery raise on an
|
||||
/// otherwise-unchanged Healthy driver leaves all four identical, so unless the rediscovery timestamp
|
||||
/// is part of the fingerprint the dedup swallows the very publish carrying the signal and the operator
|
||||
/// never sees the prompt.
|
||||
/// <para>Positive control: the assertion is that the count STRICTLY INCREASES across the raise. An
|
||||
/// "eventually non-null" assertion alone would pass on the warm-up publish and prove nothing. Revert
|
||||
/// <c>_rediscoveryNeededUtc</c> out of the fingerprint tuple and this test must fail.</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Rediscovery_raise_is_not_swallowed_by_the_unchanged_health_dedup()
|
||||
{
|
||||
var driver = new RediscoverableStubDriver();
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var parent = CreateTestProbe();
|
||||
parent.IgnoreMessages(_ => true);
|
||||
var actor = parent.ChildActorOf(DriverInstanceActor.Props(driver, healthPublisher: publisher));
|
||||
|
||||
actor.Tell(new DriverInstanceActor.InitializeRequested("{}"));
|
||||
AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(0), TimeSpan.FromSeconds(3));
|
||||
|
||||
// Let the actor settle so nothing else is in flight, then capture the baseline. Everything about
|
||||
// the driver's health is now stable — only the rediscovery flag will change.
|
||||
ExpectNoMsg(TimeSpan.FromMilliseconds(200));
|
||||
var before = publisher.Published.Count;
|
||||
|
||||
driver.RaiseRediscovery("symbol-version-changed");
|
||||
|
||||
AwaitAssert(
|
||||
() => publisher.Published.Count.ShouldBeGreaterThan(before),
|
||||
TimeSpan.FromSeconds(3));
|
||||
publisher.Published[^1].RediscoveryNeededUtc.ShouldNotBeNull();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The signal is sticky: a later health publish still carries it. The remote's tag set stayed
|
||||
/// changed, and only an operator re-browsing resolves that — which this actor cannot observe. A
|
||||
/// flag that cleared itself on the next heartbeat would be a prompt the operator could miss entirely.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Rediscovery_flag_persists_on_subsequent_health_publishes()
|
||||
{
|
||||
var driver = new RediscoverableStubDriver();
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var parent = CreateTestProbe();
|
||||
parent.IgnoreMessages(_ => true);
|
||||
var actor = parent.ChildActorOf(DriverInstanceActor.Props(driver, healthPublisher: publisher));
|
||||
|
||||
actor.Tell(new DriverInstanceActor.InitializeRequested("{}"));
|
||||
AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(0), TimeSpan.FromSeconds(3));
|
||||
driver.RaiseRediscovery("agent-instance-changed");
|
||||
AwaitAssert(
|
||||
() => publisher.Published.Any(p => p.RediscoveryNeededUtc is not null),
|
||||
TimeSpan.FromSeconds(3));
|
||||
|
||||
// Force a further publish by changing something else about the driver's health.
|
||||
driver.SetLastError("transient read failure");
|
||||
AwaitAssert(
|
||||
() => publisher.Published.Any(p => p.LastError == "transient read failure"),
|
||||
TimeSpan.FromSeconds(3));
|
||||
|
||||
var latest = publisher.Published[^1];
|
||||
latest.RediscoveryNeededUtc.ShouldNotBeNull();
|
||||
latest.RediscoveryReason.ShouldBe("agent-instance-changed");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <b>Leak guard.</b> The <see cref="IDriver"/> instance can OUTLIVE the actor — the host stops a
|
||||
/// child and respawns another around the same driver object — so a missing <c>-=</c> in
|
||||
/// <c>PostStop</c> accumulates one handler per respawn, each holding a dead <c>Self</c>. Asserts the
|
||||
/// driver's invocation list is empty again after the actor stops.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Stopping_the_actor_detaches_the_rediscovery_handler()
|
||||
{
|
||||
var driver = new RediscoverableStubDriver();
|
||||
var parent = CreateTestProbe();
|
||||
parent.IgnoreMessages(_ => true);
|
||||
var actor = parent.ChildActorOf(DriverInstanceActor.Props(driver));
|
||||
|
||||
actor.Tell(new DriverInstanceActor.InitializeRequested("{}"));
|
||||
AwaitAssert(() => driver.SubscriberCount.ShouldBe(1), TimeSpan.FromSeconds(3));
|
||||
|
||||
Watch(actor);
|
||||
actor.Tell(PoisonPill.Instance);
|
||||
ExpectTerminated(actor, TimeSpan.FromSeconds(3));
|
||||
|
||||
driver.SubscriberCount.ShouldBe(0);
|
||||
}
|
||||
|
||||
/// <summary>A driver that does NOT implement <see cref="IRediscoverable"/> must publish a null flag —
|
||||
/// the field is absent, not defaulted to "needs re-browse".</summary>
|
||||
[Fact]
|
||||
public void A_non_rediscoverable_driver_never_sets_the_flag()
|
||||
{
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var parent = CreateTestProbe();
|
||||
parent.IgnoreMessages(_ => true);
|
||||
var actor = parent.ChildActorOf(DriverInstanceActor.Props(new StubDriver(), healthPublisher: publisher));
|
||||
|
||||
actor.Tell(new DriverInstanceActor.InitializeRequested("{}"));
|
||||
AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(0), TimeSpan.FromSeconds(3));
|
||||
|
||||
publisher.Published.ShouldAllBe(p => p.RediscoveryNeededUtc == null);
|
||||
}
|
||||
|
||||
/// <summary>Captures every health publish so a test can assert on the rediscovery fields.</summary>
|
||||
private sealed record HealthPublish(
|
||||
string ClusterId,
|
||||
string DriverInstanceId,
|
||||
DriverHealth Health,
|
||||
int ErrorCount5Min,
|
||||
DateTime? RediscoveryNeededUtc,
|
||||
string? RediscoveryReason)
|
||||
{
|
||||
public string? LastError => Health.LastError;
|
||||
}
|
||||
|
||||
private sealed class RecordingHealthPublisher : IDriverHealthPublisher
|
||||
{
|
||||
private readonly List<HealthPublish> _published = [];
|
||||
|
||||
/// <summary>Thread-safe snapshot — <c>Publish</c> is called from the actor thread while the test
|
||||
/// asserts from its own.</summary>
|
||||
public IReadOnlyList<HealthPublish> Published
|
||||
{
|
||||
get { lock (_published) return _published.ToArray(); }
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public void Publish(
|
||||
string clusterId,
|
||||
string driverInstanceId,
|
||||
DriverHealth health,
|
||||
int errorCount5Min,
|
||||
DateTime? rediscoveryNeededUtc = null,
|
||||
string? rediscoveryReason = null)
|
||||
{
|
||||
lock (_published)
|
||||
{
|
||||
_published.Add(new HealthPublish(
|
||||
clusterId, driverInstanceId, health, errorCount5Min, rediscoveryNeededUtc, rediscoveryReason));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A stub driver exposing <see cref="IRediscoverable"/>, a hook to raise it, and the live subscriber
|
||||
/// count (for the detach guard).
|
||||
/// <para><b>Implemented from scratch rather than derived from the shared <c>StubDriver</c> on
|
||||
/// purpose.</b> That one returns <c>GetHealth() => new(Healthy, DateTime.UtcNow, null)</c>, so its
|
||||
/// health fingerprint differs on EVERY call and the dedup under test never engages — which would make
|
||||
/// <see cref="Rediscovery_raise_is_not_swallowed_by_the_unchanged_health_dedup"/> pass vacuously
|
||||
/// whether or not the fix is present. This stub's health is STABLE until a test changes it.</para>
|
||||
/// </summary>
|
||||
private sealed class RediscoverableStubDriver : IDriver, IRediscoverable
|
||||
{
|
||||
private static readonly DateTime FixedLastRead = new(2026, 7, 27, 12, 0, 0, DateTimeKind.Utc);
|
||||
private volatile string? _lastError;
|
||||
|
||||
/// <inheritdoc />
|
||||
public event EventHandler<RediscoveryEventArgs>? OnRediscoveryNeeded;
|
||||
|
||||
/// <inheritdoc />
|
||||
public string DriverInstanceId => "rediscoverable-stub-1";
|
||||
|
||||
/// <inheritdoc />
|
||||
public string DriverType => "Stub";
|
||||
|
||||
/// <summary>Number of live subscribers on <see cref="OnRediscoveryNeeded"/>.</summary>
|
||||
public int SubscriberCount => OnRediscoveryNeeded?.GetInvocationList().Length ?? 0;
|
||||
|
||||
/// <summary>Raises the event exactly as a real driver's watcher does.</summary>
|
||||
public void RaiseRediscovery(string reason)
|
||||
=> OnRediscoveryNeeded?.Invoke(this, new RediscoveryEventArgs(reason, ScopeHint: null));
|
||||
|
||||
/// <summary>Changes the reported health so a test can force a publish that is NOT the rediscovery one.</summary>
|
||||
public void SetLastError(string error) => _lastError = error;
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) => Task.CompletedTask;
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken) => Task.CompletedTask;
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task ShutdownAsync(CancellationToken cancellationToken) => Task.CompletedTask;
|
||||
|
||||
/// <inheritdoc />
|
||||
public DriverHealth GetHealth() => new(DriverState.Healthy, FixedLastRead, _lastError);
|
||||
|
||||
/// <inheritdoc />
|
||||
public long GetMemoryFootprint() => 0;
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task FlushOptionalCachesAsync(CancellationToken cancellationToken) => Task.CompletedTask;
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user