feat(drivers): tag-set drift detection, gated on SupportsOnlineDiscovery
The second thing #516/§8.2 deliberately did not build. The original objection stands — Modbus, S7, MQTT, AbLegacy and Sql echo authored config from DiscoverAsync, so diffing discovered against authored is a tautology for them — but it is now ENCODED rather than used as a reason not to build. The discriminator already existed: ITagDiscovery.SupportsOnlineDiscovery is documented as "enumerates the tag set from the live backend rather than replaying pre-declared/authored tags", and it separates the fleet cleanly — FOCAS, TwinCAT, MTConnect and AbCip true; the five config-echo drivers explicitly false. The check runs only for a driver that is SupportsOnlineDiscovery AND reports the new AuthoredDiscoveryRefs. That is nullable rather than empty-by-default on purpose: an empty collection legitimately means "nothing authored, everything discovered is new", so conflating the two would report total drift for a driver that never opted in. Where the value is: AbCip and FOCAS browse a live backend but implement no IRediscoverable, so before this they had no change signal at all. A periodic compare is the only way to notice a PLC re-download. A finding that changed the design: FOCAS, TwinCAT and AbCip all re-emit their authored tags into the same DiscoverAsync stream as device-derived ones, so the browse picker can show an operator their existing tags. Discovered is therefore always a superset of authored, and a VANISHED tag can never appear missing — one whole direction is structurally undetectable for three of the four. Shipping a Vanished list that is permanently empty for them would have been the half-inert seam this whole exercise removes. So the driver declares DiscoveryStreamIncludesAuthoredTags, the detector does not compute the undetectable half, and the operator message says "missing-tag detection unavailable for this driver" rather than letting the absence of a missing-tag clause read as reassurance. MTConnect is the only driver where both directions work — its stream is purely probe-model-derived. Behaviour: a failed browse is not drift (an unreachable device would otherwise report every authored tag as vanished); a truncated capture is skipped for the same reason; an unchanged drift is reported once rather than re-stamping its timestamp; drift that resolves clears the prompt. Interval defaults to 5 minutes — it browses a real device, and a tag set changing is an engineering event, not a runtime one. It reuses the Stage-2 surfacing, raising the same /hosts re-browse prompt, and never rebuilds the served address space. Comparison logic is a pure, separately-tested type rather than actor-inline. 14 new tests. The gate was verified load-bearing by deleting the SupportsOnlineDiscovery half: the tautology-driver test goes red, and it asserts discovery was never invoked rather than merely "no prompt raised", which would have passed for the wrong reason if the timer never fired. Full suite green except the same three pre-existing fixture-gated integration suites, identical counts.
This commit is contained in:
Joseph Doherty
+268
@@ -0,0 +1,268 @@
|
||||
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>
|
||||
/// The tag-set drift check: periodically re-browse a genuinely-browsable driver and compare what the
|
||||
/// remote offers against what an operator authored.
|
||||
/// <para><b>Why this is gated rather than run for every driver.</b> Modbus, S7, AbLegacy, Sql and Mqtt
|
||||
/// all stream their AUTHORED tags back out of <c>DiscoverAsync</c> rather than enumerating the device, so
|
||||
/// diffing the two sets for them is a tautology that reports "no drift" forever. A check that cannot fail
|
||||
/// is worse than no check, because it reads as coverage — so the actor runs this only for drivers that
|
||||
/// declare <c>SupportsOnlineDiscovery</c> AND report <c>AuthoredDiscoveryRefs</c>.</para>
|
||||
/// <para>The signal is advisory and shares the Stage-2 surfacing: it raises the same <c>/hosts</c>
|
||||
/// re-browse prompt an <see cref="IRediscoverable"/> raise does. The served address space is never
|
||||
/// rebuilt — raw tags are authored through the <c>/raw</c> browse-commit flow.</para>
|
||||
/// </summary>
|
||||
[Trait("Category", "Unit")]
|
||||
public sealed class DriverInstanceActorDriftCheckTests : RuntimeActorTestBase
|
||||
{
|
||||
private static readonly TimeSpan Tick = TimeSpan.FromMilliseconds(60);
|
||||
private static readonly TimeSpan Budget = TimeSpan.FromSeconds(3);
|
||||
|
||||
/// <summary>A device offering a tag nothing binds raises the operator prompt, naming what appeared.</summary>
|
||||
[Fact]
|
||||
public void A_new_ref_on_the_device_raises_the_re_browse_prompt()
|
||||
{
|
||||
var driver = new BrowsableStubDriver(discovered: ["a", "b", "c"], authored: ["a", "b"]);
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var actor = SpawnConnected(driver, publisher);
|
||||
|
||||
AwaitAssert(
|
||||
() =>
|
||||
{
|
||||
var signalled = publisher.Published.LastOrDefault(p => p.RediscoveryNeededUtc is not null);
|
||||
signalled.ShouldNotBeNull();
|
||||
signalled!.RediscoveryReason.ShouldNotBeNull();
|
||||
signalled.RediscoveryReason!.ShouldContain("1 new on device");
|
||||
signalled.RediscoveryReason!.ShouldContain("c");
|
||||
},
|
||||
Budget);
|
||||
|
||||
Sys.Stop(actor);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <b>The gate that makes this feature honest.</b> A driver whose <c>DiscoverAsync</c> replays
|
||||
/// authored config rather than browsing the device declares <c>SupportsOnlineDiscovery == false</c>,
|
||||
/// and must never be drift-checked — for it the comparison could only ever say "no drift".
|
||||
/// <para>Positive control: the driver is handed a set that WOULD read as drift, and the assertion is
|
||||
/// that discovery is never even invoked. Asserting only "no prompt raised" would pass for the wrong
|
||||
/// reason (e.g. if the timer never fired at all).</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void A_driver_that_replays_authored_config_is_never_drift_checked()
|
||||
{
|
||||
var driver = new BrowsableStubDriver(
|
||||
discovered: ["a", "b", "c"], authored: ["a"], supportsOnlineDiscovery: false);
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var actor = SpawnConnected(driver, publisher);
|
||||
|
||||
// Long enough for several ticks had the timer been scheduled.
|
||||
ExpectNoMsg(TimeSpan.FromMilliseconds(400));
|
||||
|
||||
driver.DiscoverCount.ShouldBe(
|
||||
0,
|
||||
"a driver that replays authored config was browsed for drift — the comparison is a tautology "
|
||||
+ "for it and would report 'no drift' forever, which reads as coverage");
|
||||
publisher.Published.ShouldAllBe(p => p.RediscoveryNeededUtc == null);
|
||||
|
||||
Sys.Stop(actor);
|
||||
}
|
||||
|
||||
/// <summary>A driver that browses but does NOT report its authored refs opts out — the default. An empty
|
||||
/// collection would mean "nothing authored, everything is new", so null must not be read that way.</summary>
|
||||
[Fact]
|
||||
public void A_driver_that_reports_no_authored_refs_is_never_drift_checked()
|
||||
{
|
||||
var driver = new BrowsableStubDriver(discovered: ["a", "b"], authored: null);
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var actor = SpawnConnected(driver, publisher);
|
||||
|
||||
ExpectNoMsg(TimeSpan.FromMilliseconds(400));
|
||||
|
||||
driver.DiscoverCount.ShouldBe(0);
|
||||
publisher.Published.ShouldAllBe(p => p.RediscoveryNeededUtc == null);
|
||||
|
||||
Sys.Stop(actor);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An UNCHANGED drift is reported once, not re-stamped on every check — a prompt whose timestamp
|
||||
/// keeps moving reads as "it just happened again" rather than "it is still true".
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void An_unchanged_drift_is_reported_once()
|
||||
{
|
||||
var driver = new BrowsableStubDriver(discovered: ["a", "b"], authored: ["a"]);
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var actor = SpawnConnected(driver, publisher);
|
||||
|
||||
AwaitAssert(
|
||||
() => publisher.Published.Count(p => p.RediscoveryNeededUtc is not null).ShouldBe(1),
|
||||
Budget);
|
||||
|
||||
// Several more ticks pass with the same drift.
|
||||
AwaitAssert(() => driver.DiscoverCount.ShouldBeGreaterThan(3), Budget);
|
||||
|
||||
publisher.Published.Count(p => p.RediscoveryNeededUtc is not null).ShouldBe(
|
||||
1,
|
||||
"an unchanged drift was re-announced; the operator prompt would keep re-stamping its timestamp");
|
||||
|
||||
Sys.Stop(actor);
|
||||
}
|
||||
|
||||
/// <summary>A failed browse is not drift. A device that is merely unreachable already shows on the health
|
||||
/// surface; reporting drift would turn every comms blip into "all your tags vanished".</summary>
|
||||
[Fact]
|
||||
public void A_failed_browse_is_not_reported_as_drift()
|
||||
{
|
||||
var driver = new BrowsableStubDriver(discovered: ["a"], authored: ["a", "b"]) { DiscoverThrows = true };
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var actor = SpawnConnected(driver, publisher);
|
||||
|
||||
AwaitAssert(() => driver.DiscoverCount.ShouldBeGreaterThan(1), Budget);
|
||||
|
||||
publisher.Published.ShouldAllBe(
|
||||
p => p.RediscoveryNeededUtc == null,
|
||||
"a browse that threw was treated as drift — an unreachable device would report every authored "
|
||||
+ "tag as vanished");
|
||||
|
||||
Sys.Stop(actor);
|
||||
}
|
||||
|
||||
/// <summary>Drift that resolves — the operator re-browsed and committed — clears the prompt, so the next
|
||||
/// genuine drift re-raises with a fresh timestamp instead of being deduped against the stale one.</summary>
|
||||
[Fact]
|
||||
public void Drift_that_resolves_clears_the_prompt()
|
||||
{
|
||||
var driver = new BrowsableStubDriver(discovered: ["a", "b"], authored: ["a"]);
|
||||
var publisher = new RecordingHealthPublisher();
|
||||
var actor = SpawnConnected(driver, publisher);
|
||||
|
||||
AwaitAssert(
|
||||
() => publisher.Published.Any(p => p.RediscoveryNeededUtc is not null),
|
||||
Budget);
|
||||
|
||||
// The operator commits the new tag: authored now matches the device.
|
||||
driver.SetAuthored(["a", "b"]);
|
||||
|
||||
AwaitAssert(
|
||||
() => publisher.Published[^1].RediscoveryNeededUtc.ShouldBeNull(),
|
||||
Budget);
|
||||
|
||||
Sys.Stop(actor);
|
||||
}
|
||||
|
||||
private IActorRef SpawnConnected(IDriver driver, IDriverHealthPublisher publisher)
|
||||
{
|
||||
var parent = CreateTestProbe();
|
||||
parent.IgnoreMessages(_ => true);
|
||||
var actor = parent.ChildActorOf(DriverInstanceActor.Props(
|
||||
driver, healthPublisher: publisher, driftCheckInterval: Tick));
|
||||
actor.Tell(new DriverInstanceActor.InitializeRequested("{}"));
|
||||
return actor;
|
||||
}
|
||||
|
||||
/// <summary>Captured health publishes, so a test can read the rediscovery fields.</summary>
|
||||
private sealed record HealthPublish(DateTime? RediscoveryNeededUtc, string? RediscoveryReason);
|
||||
|
||||
private sealed class RecordingHealthPublisher : IDriverHealthPublisher
|
||||
{
|
||||
private readonly List<HealthPublish> _published = [];
|
||||
|
||||
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(rediscoveryNeededUtc, rediscoveryReason));
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A driver that browses a fake "device". Health is STABLE so the publish dedup genuinely engages —
|
||||
/// the shared <c>StubDriver</c> returns <c>DateTime.UtcNow</c>, which would make every publish look
|
||||
/// changed and let a dedup-related bug pass unnoticed.
|
||||
/// </summary>
|
||||
private sealed class BrowsableStubDriver(
|
||||
string[] discovered,
|
||||
string[]? authored,
|
||||
bool supportsOnlineDiscovery = true) : IDriver, ITagDiscovery
|
||||
{
|
||||
private static readonly DateTime FixedLastRead = new(2026, 7, 28, 9, 0, 0, DateTimeKind.Utc);
|
||||
private volatile string[]? _authored = authored;
|
||||
private int _discoverCount;
|
||||
|
||||
/// <summary>Number of discovery passes the actor has driven — the direct read of whether the drift
|
||||
/// check ran at all.</summary>
|
||||
public int DiscoverCount => Volatile.Read(ref _discoverCount);
|
||||
|
||||
/// <summary>When true, every browse throws — models an unreachable device.</summary>
|
||||
public bool DiscoverThrows { get; init; }
|
||||
|
||||
/// <inheritdoc />
|
||||
public string DriverInstanceId => "browsable-stub-1";
|
||||
|
||||
/// <inheritdoc />
|
||||
public string DriverType => "Stub";
|
||||
|
||||
/// <inheritdoc />
|
||||
public bool SupportsOnlineDiscovery => supportsOnlineDiscovery;
|
||||
|
||||
/// <inheritdoc />
|
||||
public IReadOnlyCollection<string>? AuthoredDiscoveryRefs => _authored;
|
||||
|
||||
/// <summary>Models the operator committing (or removing) tags between checks.</summary>
|
||||
public void SetAuthored(string[] refs) => _authored = refs;
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task DiscoverAsync(IAddressSpaceBuilder builder, CancellationToken cancellationToken)
|
||||
{
|
||||
Interlocked.Increment(ref _discoverCount);
|
||||
if (DiscoverThrows) throw new IOException("device unreachable");
|
||||
|
||||
var folder = builder.Folder("Stub", "Stub");
|
||||
foreach (var r in discovered)
|
||||
{
|
||||
folder.Variable(r, r, new DriverAttributeInfo(
|
||||
FullName: r,
|
||||
DriverDataType: DriverDataType.Float64,
|
||||
IsArray: false,
|
||||
ArrayDim: null,
|
||||
SecurityClass: SecurityClassification.ViewOnly,
|
||||
IsHistorized: false));
|
||||
}
|
||||
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
|
||||
/// <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, null);
|
||||
|
||||
/// <inheritdoc />
|
||||
public long GetMemoryFootprint() => 0;
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task FlushOptionalCachesAsync(CancellationToken cancellationToken) => Task.CompletedTask;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,113 @@
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Runtime.Drivers;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers;
|
||||
|
||||
/// <summary>
|
||||
/// The pure half of tag-set drift detection: what the remote offers vs. what an operator authored.
|
||||
/// Kept out of the actor so the comparison rules are testable without an actor system.
|
||||
/// </summary>
|
||||
[Trait("Category", "Unit")]
|
||||
public sealed class TagSetDriftDetectorTests
|
||||
{
|
||||
/// <summary>Matching sets are not drift, and must not raise an operator prompt.</summary>
|
||||
[Fact]
|
||||
public void Identical_sets_are_not_drift()
|
||||
{
|
||||
var drift = TagSetDriftDetector.Compare(["a", "b"], ["b", "a"]);
|
||||
|
||||
drift.HasDrift.ShouldBeFalse();
|
||||
drift.Describe().ShouldBe("no drift");
|
||||
}
|
||||
|
||||
/// <summary>A tag the device now offers that nothing binds — the common case after a PLC re-download.</summary>
|
||||
[Fact]
|
||||
public void A_ref_on_the_device_that_is_not_authored_is_reported_as_appeared()
|
||||
{
|
||||
var drift = TagSetDriftDetector.Compare(["a", "b", "c"], ["a", "b"]);
|
||||
|
||||
drift.Appeared.ShouldBe(["c"]);
|
||||
drift.Vanished.ShouldBeEmpty();
|
||||
drift.HasDrift.ShouldBeTrue();
|
||||
}
|
||||
|
||||
/// <summary>An authored tag the device no longer offers — detectable only for a driver whose discovery
|
||||
/// stream is purely device-derived.</summary>
|
||||
[Fact]
|
||||
public void An_authored_ref_missing_from_the_device_is_reported_as_vanished()
|
||||
{
|
||||
var drift = TagSetDriftDetector.Compare(["a"], ["a", "b"]);
|
||||
|
||||
drift.Vanished.ShouldBe(["b"]);
|
||||
drift.Appeared.ShouldBeEmpty();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <b>The honesty guard.</b> When a driver re-emits its authored tags into the discovery stream —
|
||||
/// FOCAS, TwinCAT and AbCip all do — the discovered set always contains the authored set, so a
|
||||
/// vanished tag is structurally invisible. The detector must not compute a misleadingly-empty
|
||||
/// Vanished list; it must flag that the direction is undetectable, and say so in the description.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void When_vanished_is_undetectable_it_is_declared_rather_than_reported_as_empty()
|
||||
{
|
||||
// "b" is authored and absent from the device — but this driver would have re-emitted it, so its
|
||||
// absence here cannot occur in practice and must not be presented as a clean result either way.
|
||||
var drift = TagSetDriftDetector.Compare(["a", "c"], ["a", "b"], detectVanished: false);
|
||||
|
||||
drift.VanishedDetectable.ShouldBeFalse();
|
||||
drift.Vanished.ShouldBeEmpty();
|
||||
drift.Appeared.ShouldBe(["c"]);
|
||||
drift.Describe().ShouldContain(
|
||||
"missing-tag detection unavailable",
|
||||
customMessage:
|
||||
"a driver that cannot see vanished tags must SAY so — otherwise the absence of a 'missing' "
|
||||
+ "clause reads as 'nothing is missing', which is the reassurance this whole exercise removes");
|
||||
}
|
||||
|
||||
/// <summary>Comparison is ordinal: a device legitimately exposing both <c>Speed</c> and <c>speed</c>
|
||||
/// must not have them collapsed, which would hide a genuinely new tag.</summary>
|
||||
[Fact]
|
||||
public void Comparison_is_case_sensitive()
|
||||
{
|
||||
var drift = TagSetDriftDetector.Compare(["Speed", "speed"], ["Speed"]);
|
||||
|
||||
drift.Appeared.ShouldBe(["speed"]);
|
||||
}
|
||||
|
||||
/// <summary>The signature identifies a drift so an UNCHANGED one is not re-announced every check.
|
||||
/// Enumeration order must not make an identical drift look new.</summary>
|
||||
[Fact]
|
||||
public void Signature_is_stable_across_enumeration_order()
|
||||
{
|
||||
var a = TagSetDriftDetector.Compare(["x", "y", "z"], ["x"]);
|
||||
var b = TagSetDriftDetector.Compare(["z", "y", "x"], ["x"]);
|
||||
|
||||
a.Signature.ShouldBe(b.Signature);
|
||||
}
|
||||
|
||||
/// <summary>A different drift must get a different signature, or the second one is never reported.</summary>
|
||||
[Fact]
|
||||
public void Signature_changes_when_the_drift_changes()
|
||||
{
|
||||
var a = TagSetDriftDetector.Compare(["x", "y"], ["x"]);
|
||||
var b = TagSetDriftDetector.Compare(["x", "y", "z"], ["x"]);
|
||||
|
||||
a.Signature.ShouldNotBe(b.Signature);
|
||||
}
|
||||
|
||||
/// <summary>The description names a couple of refs and then counts, so swapping a whole PLC program
|
||||
/// yields a readable message rather than a wall of tag names.</summary>
|
||||
[Fact]
|
||||
public void Describe_samples_rather_than_listing_everything()
|
||||
{
|
||||
var discovered = Enumerable.Range(0, 50).Select(i => $"tag{i:D2}").ToArray();
|
||||
var drift = TagSetDriftDetector.Compare(discovered, []);
|
||||
|
||||
var text = drift.Describe();
|
||||
text.ShouldContain("50 new on device");
|
||||
text.ShouldContain("+48 more");
|
||||
text.Length.ShouldBeLessThan(120);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user