From 610306734cdc176b574f3304fe8ce5fb4a5559dc Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:51:16 -0400 Subject: [PATCH] fix(r2-04): Galaxy fail-closed write when AdviseSupervisory fails - Bad status feeds #5 node revert (06/S-1) --- ...2-04-failure-visibility-plan.md.tasks.json | 2 +- .../Runtime/GatewayGalaxyDataWriter.cs | 29 +++++++++--- .../GatewayGalaxyDataWriterFailClosedTests.cs | 45 +++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json b/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json index 8ff30a3e..9e7cacfd 100644 --- a/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json +++ b/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json @@ -7,7 +7,7 @@ { "id": "T3", "subject": "Materialise* passes return swallowed-failure counts (void -> int, five methods)", "status": "completed", "blockedBy": ["T2"] }, { "id": "T4", "subject": "OpcUaApplyFailed counter + OpcUaPublishActor Error/meter surfacing of degraded applies", "status": "completed", "blockedBy": ["T3"] }, { "id": "T5", "subject": "Galaxy internal IMxWriteOps seam under GatewayGalaxyDataWriter (sealed SDK session untestable) (06/S-1)", "status": "completed", "blockedBy": [] }, - { "id": "T6", "subject": "Galaxy fail-closed write on AdviseSupervisory failure -> BadCommunicationError, no raw write", "status": "pending", "blockedBy": ["T5"] }, + { "id": "T6", "subject": "Galaxy fail-closed write on AdviseSupervisory failure -> BadCommunicationError, no raw write", "status": "completed", "blockedBy": ["T5"] }, { "id": "T7", "subject": "Galaxy counters galaxy.writes.advise_failed + galaxy.writes.unconfirmed (empty-statuses Good metered)", "status": "pending", "blockedBy": ["T6"] }, { "id": "T8", "subject": "Galaxy skip-gated live smoke: normal advised write still returns Good post-change", "status": "pending", "blockedBy": ["T6"] }, { "id": "T9", "subject": "PrimaryGatePolicy pure class + truth-table tests + otopcua.redundancy.primary_gate_denied counter (03/S4)", "status": "pending", "blockedBy": [] }, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyDataWriter.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyDataWriter.cs index 67bb23e3..7374ec12 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyDataWriter.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyDataWriter.cs @@ -178,8 +178,16 @@ public sealed class GatewayGalaxyDataWriter : IGalaxyDataWriter // A raw Write runs with NO user login (WriteUserId is typically 0), so MXAccess // only COMMITS the value when the item is advised in SUPERVISORY mode. Without it // the gateway's Write call doesn't throw (reply looks OK) but the value never - // reaches the galaxy. AdviseSupervisory once per handle, then Write. - await EnsureSupervisoryAdvisedAsync(ops, itemHandle, ct).ConfigureAwait(false); + // reaches the galaxy — a knowingly-lost write. FAIL-CLOSED (archreview 06/S-1): when + // AdviseSupervisory fails we do NOT issue the raw Write; we return Bad so the + // #5 write-outcome self-correction reverts the phantom-Good node instead of leaving it. + // The real fix is a gateway-side WriteComplete correlation (mxaccessgw backlog) so the + // unary reply's Statuses carry the actual COM commit outcome; until then advise-status + + // the galaxy.writes.* meters are the honest signal. + if (!await EnsureSupervisoryAdvisedAsync(ops, itemHandle, ct).ConfigureAwait(false)) + { + return new WriteResult(StatusCodeMap.BadCommunicationError); + } reply = await ops.WriteRawAsync(itemHandle, mxValue, _writeUserId, ct) .ConfigureAwait(false); } @@ -226,10 +234,16 @@ public sealed class GatewayGalaxyDataWriter : IGalaxyDataWriter /// session reconnect via , so a fresh session always /// re-advises — the supervisory state never outlives the handle it was taken against. /// - private async Task EnsureSupervisoryAdvisedAsync( + /// Ensure the item is supervisory-advised so a no-login raw Write can commit. Returns + /// true when the item is advised (already-advised this session, or the advise round-trip + /// succeeded), false when the advise failed — the caller then FAILS THE WRITE CLOSED + /// (archreview 06/S-1) rather than issuing a raw Write that cannot commit. + /// true when advised; false when the advise failed (caller must not write). + private async Task EnsureSupervisoryAdvisedAsync( IMxWriteOps ops, int itemHandle, CancellationToken ct) { - if (!_supervisedHandles.TryAdd(itemHandle, 0)) return; + // Already advised this session (a prior successful advise on this handle) — nothing to do. + if (!_supervisedHandles.TryAdd(itemHandle, 0)) return true; var request = new MxCommandRequest { @@ -249,13 +263,16 @@ public sealed class GatewayGalaxyDataWriter : IGalaxyDataWriter var reply = await ops.InvokeAsync(request, ct).ConfigureAwait(false); if (reply.ProtocolStatus is { } proto && proto.Code != ProtocolStatusCode.Ok) { - // Supervisory advise failed — forget it so the next write retries, and let the - // write proceed (it surfaces its own status via TranslateReply). + // Supervisory advise failed — forget it so the next write retries the advise, and report + // failure so the caller fails the write closed (the raw Write would not commit). _supervisedHandles.TryRemove(itemHandle, out _); _logger.LogWarning( "GalaxyDriver supervisory advise failed for item {ItemHandle}: {Code} {Message}", itemHandle, proto.Code, proto.Message); + return false; } + + return true; } /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/GatewayGalaxyDataWriterFailClosedTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/GatewayGalaxyDataWriterFailClosedTests.cs index ec3c375f..fe4eea7b 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/GatewayGalaxyDataWriterFailClosedTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/GatewayGalaxyDataWriterFailClosedTests.cs @@ -36,6 +36,51 @@ public sealed class GatewayGalaxyDataWriterFailClosedTests ops.RawWriteHandles.ShouldBeEmpty(); } + // ---------------- T6: fail-closed on advise failure ---------------- + + /// When AdviseSupervisory fails (non-OK protocol status), the raw Write must NOT be issued and + /// the writer returns Bad (which fires the #5 node revert). The handle must not linger in the supervised + /// cache so the next write retries the advise. + [Fact] + public async Task WriteOne_AdviseFails_ReturnsBadCommunicationError_AndNeverIssuesRawWrite() + { + var ops = new FakeMxWriteOps + { + InvokeResponder = _ => new MxCommandReply + { + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.MxaccessFailure, Message = "advise boom" }, + }, + }; + var writer = NewWriter(); + + var result = await writer.WriteOneForTestAsync( + ops, new WriteRequest("Tag.A", true), SecurityClassification.FreeAccess, CancellationToken.None); + + result.StatusCode.ShouldBe(StatusCodeMap.BadCommunicationError); + ops.Invokes.Count(r => r.Command.Kind == MxCommandKind.AdviseSupervisory).ShouldBe(1); + ops.RawWriteHandles.ShouldBeEmpty(); // the knowingly-lost raw write was NOT issued + writer.CachedSupervisedHandleCount.ShouldBe(0); // advise failure forgot the handle → next write retries + } + + /// When advise succeeds, the raw Write proceeds and a clean reply returns Good (regression). + [Fact] + public async Task WriteOne_AdviseSucceeds_RawWriteProceeds() + { + var ops = new FakeMxWriteOps + { + InvokeResponder = _ => new MxCommandReply { ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok } }, + }; + var writer = NewWriter(); + + var result = await writer.WriteOneForTestAsync( + ops, new WriteRequest("Tag.B", 42), SecurityClassification.FreeAccess, CancellationToken.None); + + result.StatusCode.ShouldBe(StatusCodeMap.Good); + ops.Invokes.Count(r => r.Command.Kind == MxCommandKind.AdviseSupervisory).ShouldBe(1); + ops.RawWriteHandles.ShouldHaveSingleItem(); // the raw write was issued after the successful advise + writer.CachedSupervisedHandleCount.ShouldBe(1); + } + // ---------------- fakes ---------------- /// A fake that records every call and returns canned replies.