fix(management): CAS-guard secured-write reject (WHERE Status='Pending') so reject can never falsify an executed two-person write (arch-review 2026-08-19 P6-1 High)

Reject read the row as Pending, then landed a blind full-row UpdateAsync. A concurrent approve could win its own Pending->Approved CAS in that gap and enter the multi-second WriteTagAsync relay; the reject's update then stamped Status/ExecutedAtUtc/ExecutionError to Rejected mid-relay, so the two-person-control record said Rejected for a write that physically executed at the plant. Works cross-node (both centrals share one MS SQL). - ISecuredWriteRepository/SecuredWriteRepository: new TryMarkRejectedAsync, mirroring TryMarkApprovedAsync exactly (conditional UPDATE ... WHERE Status='Pending', writes only the columns the reject transition owns). No schema change, no migration, no rowversion column. - HandleRejectSecuredWrite: uses the CAS; on loss re-reads the row and returns the status that actually won (same UX shape as approve's lost-race error), and emits the SecuredWriteReject audit row ONLY on a won CAS. - Audited the other lifecycle writes: approve/reject/expiry (inline + the opportunistic list sweep) now ALL transition only out of Pending, so winning the approve CAS gives that leg exclusive ownership of the row and its post-relay Executed/Failed update stays safe as an unconditional write. Documented in place. - Tests: repository-level approve-vs-reject races in both orders (exactly one wins, loser false, row reflects the winner only) plus reject-vs-expiry; handler-level lost-race reject (conflict error, no write, no audit row) and approve on an already-rejected row (no CAS, no relay, no audit).
2026-08-19 13:46:59 -04:00
parent 59468399db
commit 61a14bccab
6 changed files with 322 additions and 20 deletions
@@ -270,6 +270,29 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
        return row;
    }

+    /// <summary>
+    /// Arms the reject compare-and-swap to succeed (this verifier wins the
+    /// Pending-&gt;Rejected transition). NSubstitute's unarmed default for the
+    /// bool-returning CAS is <c>false</c>, i.e. "lost the race", so every reject test
+    /// that expects the decision to land must arm it explicitly (arch-review
+    /// 2026-08-19 P6-1).
+    /// </summary>
+    private void ArmRejectCasSuccess(long id) =>
+        _securedWriteRepo.TryMarkRejectedAsync(
+                id, Arg.Any<string>(), Arg.Any<string?>(), Arg.Any<DateTime>(), Arg.Any<CancellationToken>())
+            .Returns(true);
+
+    /// <summary>
+    /// Lets the best-effort audit path settle, then asserts NOTHING was written. Audit
+    /// emission is awaited inline before the handler replies, so the reply itself is the
+    /// happens-before edge; the short sleep is belt-and-braces against a late writer.
+    /// </summary>
+    private static void AssertNoAuditRows(List<AuditEvent> captured)
+    {
+        Thread.Sleep(250);
+        Assert.Empty(captured);
+    }
+
    [Fact]
    public void Reject_BySubmittingUser_ReturnsError_StatusUnchanged()
    {
@@ -284,17 +307,15 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
        _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().TryMarkRejectedAsync(
+            default, default!, default, default, default);
    }

    [Fact]
-    public void Reject_ByDifferentUser_FlipsStatusToRejected()
+    public void Reject_ByDifferentUser_CasFlipsStatusToRejected_NoBlindUpdate()
    {
        SeedPendingWrite(7, operatorUser: "alice");
-
-        PendingSecuredWrite? updated = null;
-        _securedWriteRepo
-            .When(r => r.UpdateAsync(Arg.Any<PendingSecuredWrite>(), Arg.Any<CancellationToken>()))
-            .Do(ci => updated = ci.Arg<PendingSecuredWrite>());
+        ArmRejectCasSuccess(7);

        var actor = CreateActor();
        var envelope = Envelope(new RejectSecuredWriteCommand(7, "not authorized"), "bob", "Verifier");
@@ -303,13 +324,21 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable

        var response = ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
        Assert.Equal(envelope.CorrelationId, response.CorrelationId);
-        Assert.NotNull(updated);
-        Assert.Equal("Rejected", updated!.Status);
-        Assert.Equal("bob", updated.VerifierUser);
-        Assert.Equal("not authorized", updated.VerifierComment);
-        Assert.NotNull(updated.DecidedAtUtc);
-        // Operator fields are preserved (fully-populated update).
-        Assert.Equal("alice", updated.OperatorUser);
+
+        // The decision goes through the conditional Pending->Rejected CAS, exactly once,
+        // carrying the verifier identity + comment (arch-review 2026-08-19 P6-1).
+        _securedWriteRepo.Received(1).TryMarkRejectedAsync(
+            7, "bob", "not authorized", Arg.Any<DateTime>(), Arg.Any<CancellationToken>());
+        // The blind full-row overwrite is gone — it could land mid-relay on a row a
+        // concurrent approve already owned.
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+
+        // The projected row reflects the committed decision.
+        Assert.Contains("Rejected", response.JsonData);
+        Assert.Contains("bob", response.JsonData);
+        Assert.Contains("not authorized", response.JsonData);
+        // Operator fields are preserved.
+        Assert.Contains("alice", response.JsonData);
    }

    [Fact]
@@ -317,6 +346,7 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
    {
        var row = SeedPendingWrite(7, operatorUser: "alice");
        row.Status = "Approved";
+        var captured = CaptureAuditEvents();

        var actor = CreateActor();
        var envelope = Envelope(new RejectSecuredWriteCommand(7, null), "bob", "Verifier");
@@ -326,6 +356,79 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
        _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().TryMarkRejectedAsync(
+            default, default!, default, default, default);
+        // Nothing was decided, so no Reject audit row may exist.
+        AssertNoAuditRows(captured);
+    }
+
+    [Fact]
+    public void Reject_WhenCasLosesRace_ReturnsConflict_WritesNothing_NoRejectAudit()
+    {
+        // The P6-1 race, from the reject side: the status read at the top of the handler
+        // saw Pending, but a concurrent approve won the Pending->Approved CAS and entered
+        // its multi-second WriteTagAsync relay before this reject reached the store. The
+        // reject must lose (CAS returns false), record NOTHING — no status write, no
+        // audit row — and report the status that actually won, so the two-person-control
+        // record can never say Rejected for a write that physically executed.
+        SeedPendingWrite(7, operatorUser: "alice");
+        _securedWriteRepo.TryMarkRejectedAsync(
+                7, Arg.Any<string>(), Arg.Any<string?>(), Arg.Any<DateTime>(), Arg.Any<CancellationToken>())
+            .Returns(false);
+        // First read (the handler's stale-read guard) sees Pending; the post-CAS re-read
+        // observes the status the concurrent approve committed.
+        var stalePending = new PendingSecuredWrite
+        {
+            Id = 7, SiteId = "SITE1", ConnectionName = "Mx1", TagPath = "Tag.A",
+            ValueJson = "true", ValueType = "Boolean", Status = "Pending",
+            OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow
+        };
+        var approvedByRival = new PendingSecuredWrite
+        {
+            Id = 7, SiteId = "SITE1", ConnectionName = "Mx1", TagPath = "Tag.A",
+            ValueJson = "true", ValueType = "Boolean", Status = "Approved",
+            OperatorUser = "alice", VerifierUser = "carol",
+            SubmittedAtUtc = DateTime.UtcNow, DecidedAtUtc = DateTime.UtcNow
+        };
+        _securedWriteRepo.GetAsync(7, Arg.Any<CancellationToken>())
+            .Returns(stalePending, approvedByRival);
+        var captured = CaptureAuditEvents();
+
+        var actor = CreateActor();
+        actor.Tell(Envelope(new RejectSecuredWriteCommand(7, "no"), "bob", "Verifier"));
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
+        // The conflict names the status that won the race.
+        Assert.Contains("Approved", response.Error);
+        Assert.Contains("cannot be rejected", response.Error);
+
+        _securedWriteRepo.Received(1).TryMarkRejectedAsync(
+            7, "bob", "no", Arg.Any<DateTime>(), Arg.Any<CancellationToken>());
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+        AssertNoAuditRows(captured);
+    }
+
+    [Fact]
+    public void Approve_OnAlreadyRejectedRow_ReturnsError_NoCas_NoRelay_NoAudit()
+    {
+        // The mirror case: a rejected row can never be approved/executed afterwards.
+        var row = SeedPendingWrite(7, operatorUser: "alice");
+        row.Status = "Rejected";
+        row.VerifierUser = "carol";
+        var captured = CaptureAuditEvents();
+
+        var actor = CreateActor();
+        actor.Tell(Envelope(new ApproveSecuredWriteCommand(7, "ok"), "bob", "Verifier"));
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
+        Assert.Contains("Rejected", response.Error);
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().TryMarkApprovedAsync(
+            default, default!, default, default, default);
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+        Assert.Equal(0, _comms.CallCount); // the device write is NEVER relayed
+        AssertNoAuditRows(captured);
    }

    [Fact]
@@ -503,6 +606,7 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
        _securedWriteRepo.TryMarkApprovedAsync(
                7, Arg.Any<string>(), Arg.Any<string?>(), Arg.Any<DateTime>(), Arg.Any<CancellationToken>())
            .Returns(false);
+        var captured = CaptureAuditEvents();

        var actor = CreateActor();
        var envelope = Envelope(new ApproveSecuredWriteCommand(7, "race"), "bob", "Verifier");
@@ -513,6 +617,9 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
        Assert.Contains("already decided", response.Error);
        Assert.Equal(0, _comms.CallCount);
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+        // A lost approve decides nothing, so it may leave no Approve audit row behind.
+        AssertNoAuditRows(captured);
    }

    [Fact]
@@ -751,8 +858,10 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable

        var resp = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
        Assert.Contains("expired", resp.Error, StringComparison.OrdinalIgnoreCase);
-        // The reject path never overwrites the row (the CAS expiry owns the transition).
+        // The reject path never writes the row (the CAS expiry owns the transition).
        _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().TryMarkRejectedAsync(
+            default, default!, default, default, default);
        _securedWriteRepo.Received(1).TryMarkExpiredAsync(1, Arg.Any<DateTime>(), Arg.Any<CancellationToken>());
    }

@@ -829,6 +938,7 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
    public void Reject_EmitsOneRejectAuditRow_WithVerifier()
    {
        SeedPendingWrite(7, operatorUser: "alice");
+        ArmRejectCasSuccess(7);
        var captured = CaptureAuditEvents();

        var actor = CreateActor();
@@ -1003,6 +1113,8 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
        actor.Tell(ScopedEnvelope(new RejectSecuredWriteCommand(42, "no"), "bob", ["3"], "Verifier"));
        ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
        _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+        _securedWriteRepo.DidNotReceiveWithAnyArgs().TryMarkRejectedAsync(
+            default, default!, default, default, default);
    }

    [Fact]