fix(runtime): a changed alarm set re-subscribes instead of latching (review F1)

The alarm subscribe gated on 'a handle exists', which is correct for an idempotent
re-push and wrong for a deploy that widens/narrows the set. Combined with the
spawn-time desired set, a respawning deploy could latch the PREVIOUS revision's
alarm refs for the life of the connection; sourceNodeIds-filtering drivers
(OpcUaClient/AbCip/Focas) would never deliver the new alarm.

Seen RED: LastSubscribedAlarmRefs stayed [alarmX] after a deploy widening to
[alarmX, alarmY].
This commit is contained in:
Joseph Doherty
2026-08-20 10:24:52 -04:00
parent c1ad9e24d2
commit edd01fcc34
3 changed files with 135 additions and 6 deletions
@@ -64,6 +64,11 @@ public sealed class DriverHostActorRestartTests : RuntimeActorTestBase
private const string DriverId = "DI-1";
private const string SpeedPath = "Plant/Modbus/dev1/speed";
private const string TempPath = "Plant/Modbus/dev1/temp";
private const string AlarmXPath = "Plant/Modbus/dev1/alarmX";
private const string AlarmYPath = "Plant/Modbus/dev1/alarmY";
/// <summary>TagConfig marking a raw tag as alarm-bearing (parsed by <c>TagConfigIntent</c>).</summary>
private const string AlarmConfig = """{"alarm":{"alarmType":"AlarmCondition","severity":500}}""";
/// <summary>Seeds + applies a v3 deployment whose single Modbus driver is ENABLED, so a real child is
/// spawned around a <see cref="RecordingSubscribableDriver"/>, and waits until that child has connected
@@ -285,6 +290,69 @@ public sealed class DriverHostActorRestartTests : RuntimeActorTestBase
duration: PresenceBudget);
}
/// <summary>
/// <b>Review finding F1 — the alarm subscription must FOLLOW a changed desired set, not latch.</b>
/// The value side converges by construction (<c>HandleSubscribeAsync</c> drops the prior subscription
/// before establishing the new one, and <c>StoreDesiredSubscriptions</c> is a full replace), but the
/// alarm side is guarded by <c>_alarmSubscriptionHandle is not null</c> in BOTH
/// <c>SubscribeDesiredAlarms</c> and <c>HandleSubscribeAlarmsAsync</c> — so once a handle exists it
/// was never re-established for the life of the connection.
/// <para>
/// That became reachable when every spawn started carrying a desired set: a tag-set edit forces a
/// respawn (raw tags fold into the merged <c>DriverConfig</c>, #516), and a fast-initialising
/// driver connects and subscribes the transient spawn-time set — the PREVIOUS revision's alarm
/// refs — before the bulk push lands. The bulk push then early-returns, latching the old set for
/// the connection's life. Drivers that FILTER on <c>sourceNodeIds</c> (OPC UA Client, AB CIP,
/// FOCAS) would silently never deliver the newly-added alarm.
/// </para>
/// </summary>
[Fact]
public void A_deploy_that_widens_the_alarm_set_re_subscribes_the_new_refs()
{
var db = NewInMemoryDbFactory();
var (host, factory) = DeployOneDriver(
db,
new[] { (DriverId, "Modbus", "dev1", "alarmX", "Float", false) },
new Dictionary<string, string> { ["alarmX"] = AlarmConfig });
AwaitAssert(
() =>
{
factory.Created.Count.ShouldBe(1);
factory.Created[0].LastSubscribedAlarmRefs.ShouldBe(new[] { AlarmXPath }, ignoreOrder: true);
},
duration: PresenceBudget);
// Revision B WIDENS the alarm set: X stays, Y is added.
var revB = V3ArtifactSeeder.SeedV3Deployment(
db, RevB,
new[]
{
(DriverId, "Modbus", "dev1", "alarmX", "Float", false),
(DriverId, "Modbus", "dev1", "alarmY", "Float", false),
},
Array.Empty<(string, string, string, string, string?)>(),
enabled: true,
tagConfigByTag: new Dictionary<string, string>
{
["alarmX"] = AlarmConfig,
["alarmY"] = AlarmConfig,
});
host.Tell(new Commons.Messages.Deploy.DispatchDeployment(revB, RevB, CorrelationId.NewId()));
AwaitAssert(
() =>
{
var current = factory.Created[^1];
current.LastSubscribedAlarmRefs.ShouldBe(
new[] { AlarmXPath, AlarmYPath },
ignoreOrder: true,
"the driver is still subscribed to the PREVIOUS revision's alarm refs — the alarm " +
"subscription latched on the transient spawn-time set and never followed the new one");
},
duration: PresenceBudget);
}
/// <summary>
/// <b>The repo's first genuine Akka SUPERVISION-restart test (03/U6).</b> Not a stop, not a stub: the
/// driver raises a malformed <c>OnDataChange</c>, the actor NREs on its own thread, and Akka's default