diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index 175cea79..90806428 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -151,7 +151,7 @@ The two-person authorization workflow for writes through the MxAccess Gateway. B - **RejectSecuredWriteCommand** (`Verifier` role): marks a `Pending` row `Rejected` with the verifier's reason. - **ListSecuredWritesCommand** (any of `Operator` / `Verifier` / `Administrator`): pending queue + terminal history, global or per-site. Although read-only, the history table exposes process-sensitive tag values, so it is **not** open to any authenticated user — it is gated to the two Secured Write roles plus `Administrator` (any-of), aligning with the `RequireSecuredWriteAccess` Central UI policy (arch-review UA1). -**Server-side pending TTL (arch-review S2).** A `Pending` secured write is not valid indefinitely. Both `ApproveSecuredWriteCommand` and `RejectSecuredWriteCommand` call a shared `TryExpireIfStaleAsync` helper immediately after the `Pending`-status check: if the row's `SubmittedAtUtc` is older than the configured `SecuredWritePendingTtl` (`ManagementServiceOptions`, default **24 h**), the helper atomically transitions it `Pending → Expired` via `ISecuredWriteRepository.TryMarkExpiredAsync` (compare-and-swap — no verifier recorded, `VerifierUser` stays `NULL`), emits a best-effort `SecuredWriteExpire` audit row (system actor, `Discarded` status — the closest terminal `AuditStatus`), and the command fails with an "expired" error. A stale setpoint therefore can **never** be approved or executed days later, and no `WriteTagRequest` is relayed. A non-positive TTL disables expiry. +**Server-side pending TTL (arch-review S2).** A `Pending` secured write is not valid indefinitely. Both `ApproveSecuredWriteCommand` and `RejectSecuredWriteCommand` call a shared `TryExpireIfStaleAsync` helper immediately after the `Pending`-status check: if the row's `SubmittedAtUtc` is older than the configured `SecuredWritePendingTtl` (`ManagementServiceOptions`, default **24 h**), the helper atomically transitions it `Pending → Expired` via `ISecuredWriteRepository.TryMarkExpiredAsync` (compare-and-swap — no verifier recorded, `VerifierUser` stays `NULL`), emits a best-effort `SecuredWriteExpire` audit row (system actor, `Discarded` status — the closest terminal `AuditStatus`), and the command fails with an "expired" error. A stale setpoint therefore can **never** be approved or executed days later, and no `WriteTagRequest` is relayed. A non-positive TTL disables expiry. `ListSecuredWritesCommand` additionally runs an **opportunistic sweep**: on every read it walks the returned page and calls the same helper for each `Pending` row, so overdue writes surface as `Expired` the next time anyone views the queue — no dedicated expiry-timer singleton is needed because the ManagementActor already runs on every central node and the CAS makes concurrent multi-node sweeps idempotent. ### External Systems diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index b64e72ce..618d7e64 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -1395,6 +1395,22 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var rows = await repo.QueryAsync(cmd.Status, cmd.SiteId, skip: 0, take: 200); + + // Opportunistic expiry sweep (arch-review S2): every time the list is read, walk + // the page and expire any overdue Pending row. TryExpireIfStaleAsync self-filters + // by age (fresh rows are no-ops) and CAS-guards the transition, so this is + // deliberately lazy/opportunistic — the ManagementActor already runs on every + // central node, so a dedicated expiry-timer singleton would be redundant, and the + // compare-and-swap makes two nodes sweeping the same overdue row concurrently + // harmless (exactly one wins; the loser observes false and re-projects the row's + // freshly-Expired state). The mutation lands on the in-memory row before projection, + // so the caller sees Expired in the same response. + foreach (var row in rows) + { + if (string.Equals(row.Status, "Pending", StringComparison.Ordinal)) + await TryExpireIfStaleAsync(sp, repo, row); + } + return new SecuredWriteListResult(rows.Select(ToSecuredWriteDto).ToList()); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs index 852e40be..e9c1d930 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs @@ -613,6 +613,42 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable Assert.Contains("not found", updated.ExecutionError); } + // ------------------------------------------------------------------------ + // Opportunistic expiry sweep on list — Task 16 (arch-review S2) + // ------------------------------------------------------------------------ + + [Fact] + public void List_SweepsStalePendingRows_MarksExpired_CallsTryMarkExpiredOnceForStaleOnly() + { + var fresh = new PendingSecuredWrite + { + Id = 1, SiteId = "SITE1", ConnectionName = "Mx1", TagPath = "Tag.A", + ValueJson = "true", ValueType = "Boolean", Status = "Pending", + OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow.AddHours(-1) + }; + var stale = new PendingSecuredWrite + { + Id = 2, SiteId = "SITE1", ConnectionName = "Mx1", TagPath = "Tag.B", + ValueJson = "false", ValueType = "Boolean", Status = "Pending", + OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow.AddHours(-25) // TTL default 24h + }; + _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any()) + .Returns(new List { fresh, stale }); + _securedWriteRepo.TryMarkExpiredAsync(2, Arg.Any(), Arg.Any()) + .Returns(true); + + var actor = CreateActor(); + actor.Tell(Envelope(new ListSecuredWritesCommand(null, null), "carol", "Operator")); + + var resp = ExpectMsg(TimeSpan.FromSeconds(5)); + // The stale row surfaces as Expired in the projection; the fresh row stays Pending. + Assert.Contains("\"id\":2", resp.JsonData); + Assert.Contains("Expired", resp.JsonData); + // Exactly one CAS — only the overdue row. + _securedWriteRepo.Received(1).TryMarkExpiredAsync(2, Arg.Any(), Arg.Any()); + _securedWriteRepo.DidNotReceive().TryMarkExpiredAsync(1, Arg.Any(), Arg.Any()); + } + // ------------------------------------------------------------------------ // Server-side TTL expiry — Task 15 (arch-review S2) // ------------------------------------------------------------------------