fix(s7): access-denied is an answer; review round-1 minors (R07:H4 follow-through)

I-1: a PUT/GET-disabled CPU (every read denied) stayed Faulted with LastSuccessfulRead frozen, so the comms-loss watchdog reconnected and re-subscribed it every 90 s forever. Both access-denied arms now also record an answer: Faulted is kept (worst state wins), the stamp advances, and the sticky write fault still clears only on a real success. Pinned end to end in Runtime.Tests: the real S7Driver under the real DriverInstanceActor watchdog is not reconnected across 2x the threshold; the control (every read timing out) is. Minors: - m-1: injectable pass clock (UtcNow); "LastSuccessfulRead advanced" is now asserted strictly, including the write-refusal case. - m-2: the editor uses S7TagShapeRules.IsByteAnchored instead of its own list. - m-3: a release that races an init publish also cancels the probe loop the publish started after the release took its snapshot. - m-4: a retained poll failing during a lifecycle operation writes no health. - m-5: a multi-var chunk that fell back memoises its members refused alone. - m-7: access-denied needs S7.Net's exact wording and no transport fault in the chain. - m-8: sync teardown clears the write-side Degraded owner; a sync Dispose racing a running shutdown is tested. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
2026-10-22 15:57:53 -04:00
parent 04ad774235
commit 081fa693af
9 changed files with 438 additions and 34 deletions
@@ -34,6 +34,8 @@ public sealed class S7_1500RefusalTests(Snap7ServerFixture sim)
        await using var drv = new S7Driver(options, driverInstanceId: "s7-refusal-live");
        await drv.InitializeAsync("{}", ct);
        var before = drv.GetHealth().LastSuccessfulRead!.Value;
+        // Let the wall clock move past the init stamp so "advanced" is asserted strictly (review m-1).
+        while (DateTime.UtcNow <= before) await Task.Yield();

        // Read the refused tag ALONE first: an all-refused pass is exactly the case that used to
        // freeze LastSuccessfulRead and latch Degraded.
@@ -42,7 +44,7 @@ public sealed class S7_1500RefusalTests(Snap7ServerFixture sim)
        refused[0].Value.ShouldBeNull();
        var health = drv.GetHealth();
        health.State.ShouldBe(DriverState.Healthy, "a CPU refusal is not a comms fault");
-        health.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(before);
+        health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(before, "the refusal is an answer: the stamp advances");

        // Positive control: the same connection still serves the seeded neighbour.
        var good = await drv.ReadAsync([S7_1500Profile.ProbeTag], ct);
@@ -115,6 +115,69 @@ public sealed class S7LifecycleGateTests
        (await drv.ReadAsync(["W0"], ct))[0].StatusCode.ShouldBe(0u);
    }

+    /// <summary>
+    ///     Review m-4: a retained poll that runs into a re-initialize in progress ("not initialized")
+    ///     must not write Degraded over the new attempt's Initializing. Waits on the poll's own log line
+    ///     (a counted signal), then asserts; the control is that a successful init then reports Healthy.
+    /// </summary>
+    [Fact]
+    public async Task A_retained_poll_does_not_overwrite_a_reinitialize_in_progress()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var factory = new ScriptedS7PlcFactory();
+        var secondOpenEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var releaseSecondOpen = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        factory.Open = async (index, token) =>
+        {
+            if (index != 1) return;
+            secondOpenEntered.TrySetResult();
+            await releaseSecondOpen.Task.WaitAsync(token);
+        };
+        var log = new CapturingS7Logger();
+        using var drv = new S7Driver(Options(), "s7-retained-poll", factory, log);
+        await drv.InitializeAsync("{}", ct);
+        await drv.SubscribeAsync(["W0"], TimeSpan.FromMilliseconds(100), ct);
+
+        var reinit = drv.InitializeAsync("{}", ct);
+        await secondOpenEntered.Task.WaitAsync(Bound, ct);
+
+        using (var cts = CancellationTokenSource.CreateLinkedTokenSource(ct))
+        {
+            cts.CancelAfter(Bound);
+            while (log.Entries.Count(e => e.Message.Contains("during a lifecycle operation")) < 2)
+                await Task.Delay(20, cts.Token); // condition wait on the poll's counted log line
+        }
+        drv.GetHealth().State.ShouldBe(DriverState.Initializing, "a retained poll must not write over the new attempt");
+
+        releaseSecondOpen.SetResult();
+        await reinit.WaitAsync(Bound, ct);
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy);
+    }
+
+    /// <summary>
+    ///     Review m-8: the SYNCHRONOUS <c>Dispose()</c> racing a still-running <c>ShutdownAsync</c> is safe too
+    ///     — neither throws, the superseded init publishes nothing, and the driver ends torn down.
+    /// </summary>
+    [Fact]
+    public async Task Sync_dispose_while_a_shutdown_is_still_running_is_safe()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var f = new FirstOpenBlocks();
+        var drv = new S7Driver(Options(), "s7-sync-dispose-race", f.Factory) { LifecycleGateTimeout = TimeSpan.FromSeconds(1) };
+
+        var init = drv.InitializeAsync("{}", ct);
+        await f.FirstOpenEntered.Task.WaitAsync(Bound, ct);
+
+        var shutdown = drv.ShutdownAsync(ct);   // waits on the gate the blocked init holds
+        Should.NotThrow(() => drv.Dispose());
+        await shutdown.WaitAsync(Bound, ct);
+
+        f.ReleaseFirstOpen.SetResult();
+        await Should.ThrowAsync<InvalidOperationException>(() => init.WaitAsync(Bound, ct));
+        f.Factory.Created.ShouldHaveSingleItem().Disposed.ShouldBeTrue();
+        drv.GetHealth().State.ShouldBe(DriverState.Unknown);
+    }
+
    /// <summary>
    ///     Dispose racing a shutdown that is still running is safe: neither throws, and the driver ends
    ///     torn down. The semaphores are no longer disposed, so the running shutdown cannot hit an
@@ -38,13 +38,24 @@ public sealed class S7RefusalHealthTests
            new S7TagDefinition("Good", "DB1.DBW0", S7DataType.UInt16)),
    };

-    private static async Task<(S7Driver Driver, ScriptedS7PlcFactory Factory, CapturingS7Logger Log)> StartAsync()
+    /// <summary>A stepped pass clock, so "LastSuccessfulRead advanced" is asserted STRICTLY.</summary>
+    private sealed class PassClock
+    {
+        public DateTime Now { get; private set; } = new(2026, 10, 22, 12, 0, 0, DateTimeKind.Utc);
+
+        /// <summary>Moves the clock one second on and returns the new time.</summary>
+        public DateTime Step() => Now = Now.AddSeconds(1);
+    }
+
+    private static async Task<(S7Driver Driver, ScriptedS7PlcFactory Factory, CapturingS7Logger Log, PassClock Clock)> StartAsync()
    {
        var factory = new ScriptedS7PlcFactory();
        var log = new CapturingS7Logger();
-        var drv = new S7Driver(Options(), "s7-refusal", factory, log);
+        var clock = new PassClock();
+        var drv = new S7Driver(Options(), "s7-refusal", factory, log) { UtcNow = () => clock.Now };
        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
-        return (drv, factory, log);
+        drv.GetHealth().LastSuccessfulRead.ShouldBe(clock.Now, "init stamps the pass clock");
+        return (drv, factory, log, clock);
    }

    /// <summary>A refused tag is Bad, health stays Healthy, LastSuccessfulRead advances, and it warns once.</summary>
@@ -52,20 +63,19 @@ public sealed class S7RefusalHealthTests
    public async Task A_read_refusal_is_per_tag_bad_with_no_health_impact_and_one_warning()
    {
        var ct = TestContext.Current.CancellationToken;
-        var (drv, factory, log) = await StartAsync();
+        var (drv, factory, log, clock) = await StartAsync();
        using var _ = drv;
        factory.Read = (addr, _) => addr == "DB99.DBW0" ? throw ReadRefusal() : Task.FromResult<object?>((ushort)7);

-        var before = drv.GetHealth().LastSuccessfulRead!.Value;
        for (var pass = 0; pass < 3; pass++)
        {
+            var at = clock.Step();
            var r = await drv.ReadAsync(["Missing"], ct);
            r[0].StatusCode.ShouldBe(BadDeviceFailure);
            drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a refusal is not a comms fault");
+            drv.GetHealth().LastSuccessfulRead.ShouldBe(at,
+                "the CPU answered, so the all-refused pass advances LastSuccessfulRead to the pass time");
        }
-
-        drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(before,
-            "the CPU answered, so the all-refused pass still advances LastSuccessfulRead");
        log.Warnings.Count(w => w.Contains("Missing")).ShouldBe(1, "one Warning per refused tag, not one per poll");

        // Positive control: the good tag still reads, and nothing reopened the connection.
@@ -82,21 +92,25 @@ public sealed class S7RefusalHealthTests
    public async Task A_refusal_after_a_comms_fault_restores_healthy()
    {
        var ct = TestContext.Current.CancellationToken;
-        var (drv, factory, _) = await StartAsync();
+        var (drv, factory, _, clock) = await StartAsync();
        using var __ = drv;

-        // Positive control: a genuine comms fault DOES degrade.
+        // Positive control: a genuine comms fault DOES degrade, and leaves LastSuccessfulRead frozen.
+        var initStamp = clock.Now;
+        clock.Step();
        factory.Read = (_, _) => throw new TimeoutException("frozen peer");
        await drv.ReadAsync(["Missing"], ct);
        var degraded = drv.GetHealth();
        degraded.State.ShouldBe(DriverState.Degraded);
+        degraded.LastSuccessfulRead.ShouldBe(initStamp);

+        var at = clock.Step();
        factory.Read = (addr, _) => addr == "DB99.DBW0" ? throw ReadRefusal() : Task.FromResult<object?>((ushort)7);
        var r = await drv.ReadAsync(["Missing"], ct);
        r[0].StatusCode.ShouldBe(BadDeviceFailure);
        var after = drv.GetHealth();
        after.State.ShouldBe(DriverState.Healthy);
-        after.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(degraded.LastSuccessfulRead!.Value);
+        after.LastSuccessfulRead.ShouldBe(at);
    }

    /// <summary>The ReadData-wrapped shape of a refusal is classified the same way.</summary>
@@ -104,7 +118,7 @@ public sealed class S7RefusalHealthTests
    public async Task A_read_data_wrapped_refusal_is_also_a_refusal()
    {
        var ct = TestContext.Current.CancellationToken;
-        var (drv, factory, _) = await StartAsync();
+        var (drv, factory, _, clock) = await StartAsync();
        using var __ = drv;
        factory.Read = (_, _) => throw new PlcException(ErrorCode.ReadData, ReadRefusal());

@@ -121,7 +135,7 @@ public sealed class S7RefusalHealthTests
    public async Task A_bare_access_denied_read_is_the_faulted_config_alert()
    {
        var ct = TestContext.Current.CancellationToken;
-        var (drv, factory, _) = await StartAsync();
+        var (drv, factory, _, clock) = await StartAsync();
        using var __ = drv;
        factory.Read = (_, _) => throw new Exception("Received error from PLC: Accessing object not allowed.");

@@ -130,12 +144,93 @@ public sealed class S7RefusalHealthTests
        drv.GetHealth().LastError!.ShouldContain("PUT/GET");
    }

+    /// <summary>
+    ///     Review I-1: an access-denied CPU (PUT/GET left at its factory default — EVERY read denied) is
+    ///     reachable and answering, so each pass advances LastSuccessfulRead while staying Faulted.
+    ///     Before, the pass stayed Faulted with the stamp frozen — the comms-loss watchdog's trip
+    ///     condition, so the actor reconnected + re-subscribed every 90 s, forever, which can never fix
+    ///     a PUT/GET setting. The control: a comms fault afterwards leaves the stamp frozen.
+    /// </summary>
+    [Fact]
+    public async Task An_all_access_denied_pass_stays_faulted_but_advances_last_successful_read()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var (drv, factory, _, clock) = await StartAsync();
+        using var __ = drv;
+        factory.Read = (_, _) => throw new Exception("Received error from PLC: Accessing object not allowed.");
+
+        DateTime last = default;
+        for (var pass = 0; pass < 2; pass++)
+        {
+            last = clock.Step();
+            await drv.ReadAsync(["Good", "Missing"], ct);
+            var h = drv.GetHealth();
+            h.State.ShouldBe(DriverState.Faulted, "the config alert is kept");
+            h.LastSuccessfulRead.ShouldBe(last, "the CPU answered, so the stamp advances every pass");
+        }
+
+        // Positive control: a real comms loss does NOT advance it (and the sticky Faulted outranks it).
+        clock.Step();
+        factory.Read = (_, _) => throw new TimeoutException("frozen peer");
+        await drv.ReadAsync(["Good"], ct);
+        drv.GetHealth().LastSuccessfulRead.ShouldBe(last);
+    }
+
+    /// <summary>The write-side access-denied is an answer too, and its sticky Faulted still clears only on a real success.</summary>
+    [Fact]
+    public async Task A_write_access_denied_advances_last_successful_read_and_stays_sticky()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var (drv, factory, _, clock) = await StartAsync();
+        using var __ = drv;
+        factory.Write = (_, _, _) => throw new PlcException(ErrorCode.WriteData,
+            new Exception("Received error from PLC: Accessing object not allowed."));
+
+        var at = clock.Step();
+        (await drv.WriteAsync([new WriteRequest("Good", (ushort)1)], ct))[0].StatusCode.ShouldBe(0x803D0000u);
+        drv.GetHealth().State.ShouldBe(DriverState.Faulted);
+        drv.GetHealth().LastSuccessfulRead.ShouldBe(at);
+
+        // A clean read leaves the write-side Faulted in place (sticky) …
+        clock.Step();
+        await drv.ReadAsync(["Good"], ct);
+        drv.GetHealth().State.ShouldBe(DriverState.Faulted);
+
+        // … and a real write success clears it.
+        factory.Write = (_, _, _) => Task.CompletedTask;
+        clock.Step();
+        await drv.WriteAsync([new WriteRequest("Good", (ushort)1)], ct);
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy);
+    }
+
+    /// <summary>
+    ///     Review m-7: a transport fault whose text happens to contain "not allowed" reopens (Degraded),
+    ///     and only S7.Net's exact "Accessing object not allowed" is the access-denied alert.
+    /// </summary>
+    [Fact]
+    public async Task Only_the_exact_access_denied_wording_without_a_transport_fault_is_access_denied()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var (drv, factory, _, clock) = await StartAsync();
+        using var __ = drv;
+
+        clock.Step();
+        factory.Write = (_, _, _) => throw new PlcException(ErrorCode.WriteData,
+            new System.IO.IOException("Accessing object not allowed: socket closed by peer"));
+        (await drv.WriteAsync([new WriteRequest("Good", (ushort)1)], ct))[0].StatusCode
+            .ShouldNotBe(0x803D0000u, "not the access-denied BadNotSupported");
+        drv.GetHealth().State.ShouldBe(DriverState.Degraded, "a transport fault is not a config alert");
+
+        S7Driver.IsAccessDenied(new PlcException(ErrorCode.WriteData, new Exception("operation not allowed"))).ShouldBeFalse();
+        S7Driver.IsAccessDenied(new Exception("Received error from PLC: Accessing object not allowed.")).ShouldBeTrue();
+    }
+
    /// <summary>A ReadData wrapping a socket fault is still a comms fault (Degraded), not a refusal.</summary>
    [Fact]
    public async Task A_read_data_wrapping_a_socket_fault_still_degrades()
    {
        var ct = TestContext.Current.CancellationToken;
-        var (drv, factory, _) = await StartAsync();
+        var (drv, factory, _, clock) = await StartAsync();
        using var __ = drv;
        factory.Read = (_, _) => throw new PlcException(ErrorCode.ReadData, new System.IO.IOException("connection reset"));

@@ -148,7 +243,7 @@ public sealed class S7RefusalHealthTests
    public async Task A_write_refusal_is_per_tag_bad_and_counts_as_answered()
    {
        var ct = TestContext.Current.CancellationToken;
-        var (drv, factory, log) = await StartAsync();
+        var (drv, factory, log, clock) = await StartAsync();
        using var _ = drv;
        factory.Write = (addr, _, _) => addr == "DB99.DBW0" ? throw WriteRefusal() : Task.CompletedTask;

@@ -160,12 +255,17 @@ public sealed class S7RefusalHealthTests

        for (var i = 0; i < 2; i++)
        {
+            var at = clock.Step();
            var w = await drv.WriteAsync([new WriteRequest("Missing", (ushort)1)], ct);
            w[0].StatusCode.ShouldBe(BadDeviceFailure);
+            var h = drv.GetHealth();
+            h.LastSuccessfulRead.ShouldBe(at, "a write refusal is an answer: the stamp advances strictly");
+            // The Degraded was written by the READ side (its own message), so it is the read pass's to clear.
+            h.State.ShouldBe(DriverState.Degraded);
        }

        var after = drv.GetHealth();
-        after.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(degraded.LastSuccessfulRead!.Value);
+        after.LastSuccessfulRead!.Value.ShouldBeGreaterThan(degraded.LastSuccessfulRead!.Value);
        log.Warnings.Count(m => m.Contains("Missing")).ShouldBe(1);
    }

@@ -177,7 +277,7 @@ public sealed class S7RefusalHealthTests
    public async Task A_good_write_clears_a_write_side_degraded()
    {
        var ct = TestContext.Current.CancellationToken;
-        var (drv, factory, _) = await StartAsync();
+        var (drv, factory, _, clock) = await StartAsync();
        using var _ = drv;

        factory.Write = (_, _, _) => throw new System.IO.IOException("connection reset");
@@ -139,6 +139,58 @@ public sealed class S7SpanRefusalFallbackTests
        drv.GetHealth().State.ShouldBe(DriverState.Degraded);
    }

+    /// <summary>
+    ///     Review m-5, the multi-var sibling: a leftover chunk holding one refused scalar used to fall back
+    ///     per tag on EVERY poll (1 + N requests). The member refused on its own is now memoised, so the
+    ///     next pass batches the rest and reads only the bad one alone.
+    /// </summary>
+    [Fact]
+    public async Task A_refused_multi_var_chunk_memoises_only_the_member_refused_alone()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var factory = new ScriptedS7PlcFactory();
+        var multiCalls = 0;
+        factory.ReadMulti = (items, _) =>
+        {
+            Interlocked.Increment(ref multiCalls);
+            if (items.Any(i => i.DB == 3))
+                // The live snap7 shape of a multi-var request with one refused item.
+                throw new global::S7.Net.PlcException(global::S7.Net.ErrorCode.ReadData,
+                    new global::S7.Net.PlcException(global::S7.Net.ErrorCode.WrongNumberReceivedBytes));
+            foreach (var i in items) i.Value = (ushort)5;
+            return Task.FromResult<IReadOnlyList<global::S7.Net.Types.DataItem>>(items);
+        };
+        factory.Read = (addr, _) => addr.StartsWith("DB3.", StringComparison.Ordinal)
+            ? throw new Exception("Received error from PLC: Object does not exist.")
+            : Task.FromResult<object?>((ushort)5);
+        using var drv = new S7Driver(new S7DriverOptions
+        {
+            Host = "192.0.2.1",
+            Probe = new S7ProbeOptions { Enabled = false },
+            RawTags = S7RawTags.Entries(
+                new S7TagDefinition("A", "DB1.DBW0", S7DataType.UInt16, Writable: false),
+                new S7TagDefinition("B", "DB2.DBW0", S7DataType.UInt16, Writable: false),
+                new S7TagDefinition("Bad", "DB3.DBW0", S7DataType.UInt16, Writable: false)),
+        }, "s7-mv-memo", factory);
+        await drv.InitializeAsync("{}", ct);
+        string[] refs = ["A", "B", "Bad"];
+
+        var before = factory.WireOps;
+        var r1 = await drv.ReadAsync(refs, ct);
+        r1[0].StatusCode.ShouldBe(0u);
+        r1[1].StatusCode.ShouldBe(0u);
+        r1[2].StatusCode.ShouldBe(BadDeviceFailure);
+        (factory.WireOps - before).ShouldBe(4, "pass 1: one refused multi-var, then one per-tag read each");
+
+        before = factory.WireOps;
+        var r2 = await drv.ReadAsync(refs, ct);
+        r2[0].StatusCode.ShouldBe(0u);
+        r2[1].StatusCode.ShouldBe(0u);
+        r2[2].StatusCode.ShouldBe(BadDeviceFailure);
+        (factory.WireOps - before).ShouldBe(2, "pass 2: A+B ride one multi-var, Bad reads alone");
+        Volatile.Read(ref multiCalls).ShouldBe(2);
+    }
+
    /// <summary>The memo is per generation: a re-initialize (e.g. a corrected DB) groups everything again.</summary>
    [Fact]
    public async Task Reinitialize_clears_the_do_not_group_memo()