fix(modbus): exception-safe batch wrapper; driver batch wiring consistency

2026-08-15 12:27:19 -04:00
parent f81cbabd2d
commit 5c61d0b133
7 changed files with 129 additions and 9 deletions
@@ -197,6 +197,74 @@ public sealed class ModbusBatchSubscriptionTests
        flattened.ShouldNotContain(item => Equals(item.Snapshot.Value, (short)102));
    }

+    /// <summary>
+    ///     A per-tag <see cref="ISubscribable.OnDataChange"/> subscriber that throws must not corrupt
+    ///     the poll cycle. <c>PollGroupEngine.PollOnceAsync</c> invokes the driver's onChange bridge
+    ///     UNGUARDED, so without the driver-side try/catch the throw unwinds the whole cycle before
+    ///     <c>onBatch</c> runs — and because Modbus buffers its published items across the cycle
+    ///     (the deadband-honouring buffer the tests above pin), the undrained items would silently
+    ///     MERGE into a later cycle's batch. This failure mode is unique to Modbus: the other poll
+    ///     drivers forward the engine's own per-cycle list and hold no state between cycles.
+    ///     Asserted on both the shape (one batch per cycle, three items each) and the VALUES, so a
+    ///     merged or dropped batch fails rather than coincidentally matching on count.
+    /// </summary>
+    [Fact]
+    public async Task A_throwing_per_tag_subscriber_does_not_break_or_contaminate_the_cycle_batch()
+    {
+        var (drv, fake) = NewDriver(
+            new ModbusTagDefinition("Level", ModbusRegion.HoldingRegisters, 0, ModbusDataType.Int16),
+            new ModbusTagDefinition("Temp", ModbusRegion.HoldingRegisters, 1, ModbusDataType.Int16),
+            new ModbusTagDefinition("Flow", ModbusRegion.HoldingRegisters, 2, ModbusDataType.Int16));
+        await drv.InitializeAsync("{}", CancellationToken.None);
+        fake.HoldingRegisters[0] = 100;
+        fake.HoldingRegisters[1] = 200;
+        fake.HoldingRegisters[2] = 300;
+
+        var thrown = 0;
+        var batches = new ConcurrentQueue<DataChangeBatchEventArgs>();
+        // The buggy external subscriber: every per-tag delivery faults.
+        drv.OnDataChange += (_, _) =>
+        {
+            Interlocked.Increment(ref thrown);
+            throw new InvalidOperationException("buggy per-tag subscriber");
+        };
+        ((IBatchSubscribable)drv).OnDataChangeBatch += (_, e) => batches.Enqueue(e);
+
+        var handle = await drv.SubscribeAsync(["Level", "Temp", "Flow"], TimeSpan.FromMilliseconds(100), CancellationToken.None);
+        try
+        {
+            // Cycle 1 — the initial-data push. All three tags publish despite every per-tag
+            // delivery faulting, and the cycle still reaches its batch.
+            await WaitForCountAsync(batches, 1, TimeSpan.FromSeconds(2));
+            batches.Count.ShouldBe(1, "the cycle must still reach onBatch after a subscriber throws");
+
+            // Cycle 2 — every tag changes again.
+            fake.HoldingRegisters[0] = 101;
+            fake.HoldingRegisters[1] = 201;
+            fake.HoldingRegisters[2] = 301;
+            await WaitForCountAsync(batches, 2, TimeSpan.FromSeconds(3));
+        }
+        finally
+        {
+            // Unsubscribe drains the in-flight poll loop, so the queues are stable from here.
+            await drv.UnsubscribeAsync(handle, CancellationToken.None);
+        }
+
+        var allBatches = batches.ToArray();
+        allBatches.Length.ShouldBeGreaterThanOrEqualTo(2);
+
+        allBatches[0].Items.Count.ShouldBe(3);
+        allBatches[0].Items.Select(i => i.Snapshot.Value).ShouldBe(new object[] { (short)100, (short)200, (short)300 });
+
+        // The load-bearing assertion: cycle 2's batch carries EXACTLY cycle 2's items. A cycle
+        // aborted mid-loop would have left its items in the buffer to merge in here.
+        allBatches[1].Items.Count.ShouldBe(3, "a cycle aborted before onBatch would merge its buffered items into the next batch");
+        allBatches[1].Items.Select(i => i.Snapshot.Value).ShouldBe(new object[] { (short)101, (short)201, (short)301 });
+
+        // Sanity: the throwing subscriber really was invoked for every published item.
+        thrown.ShouldBeGreaterThanOrEqualTo(6);
+    }
+
    private static async Task WaitForCountAsync<T>(ConcurrentQueue<T> q, int target, TimeSpan timeout)
    {
        var deadline = DateTime.UtcNow + timeout;