diff --git a/clients/dotnet/MxGateway.Client.Tests/FakeGatewayTransport.cs b/clients/dotnet/MxGateway.Client.Tests/FakeGatewayTransport.cs index 76f67b9..ea36dc7 100644 --- a/clients/dotnet/MxGateway.Client.Tests/FakeGatewayTransport.cs +++ b/clients/dotnet/MxGateway.Client.Tests/FakeGatewayTransport.cs @@ -91,6 +91,19 @@ internal sealed class FakeGatewayTransport(MxGatewayClientOptions options) : IMx /// public Queue CloseSessionExceptions { get; } = new(); + /// + /// Gets or sets a value indicating whether thrown s are mapped + /// to the way the production gRPC transport does. Lets + /// retry tests exercise the wrapped-exception predicate branch that runs in production. + /// + public bool MapTransportExceptions { get; set; } + + /// + /// Gets or sets an optional hook awaited inside CloseSessionAsync after the call is + /// recorded; lets tests pause a close mid-flight to observe concurrent dispose. + /// + public Func? CloseSessionHook { get; set; } + /// /// Gets the queue of exceptions to throw from InvokeAsync. /// @@ -108,7 +121,7 @@ internal sealed class FakeGatewayTransport(MxGatewayClientOptions options) : IMx OpenSessionCalls.Add((request, callOptions)); if (OpenSessionExceptions.TryDequeue(out Exception? exception)) { - throw exception; + throw Translate(exception, callOptions); } return Task.FromResult(OpenSessionReply); @@ -119,17 +132,23 @@ internal sealed class FakeGatewayTransport(MxGatewayClientOptions options) : IMx /// /// The CloseSessionRequest to process. /// Call options specifying RPC behavior. - public Task CloseSessionAsync( + public async Task CloseSessionAsync( CloseSessionRequest request, CallOptions callOptions) { CloseSessionCalls.Add((request, callOptions)); - if (CloseSessionExceptions.TryDequeue(out Exception? exception)) + + if (CloseSessionHook is not null) { - throw exception; + await CloseSessionHook().ConfigureAwait(false); } - return Task.FromResult(CloseSessionReply); + if (CloseSessionExceptions.TryDequeue(out Exception? exception)) + { + throw Translate(exception, callOptions); + } + + return CloseSessionReply; } /// @@ -144,7 +163,7 @@ internal sealed class FakeGatewayTransport(MxGatewayClientOptions options) : IMx InvokeCalls.Add((request, callOptions)); if (InvokeExceptions.TryDequeue(out Exception? exception)) { - throw exception; + throw Translate(exception, callOptions); } return Task.FromResult(_invokeReplies.Dequeue()); @@ -239,4 +258,18 @@ internal sealed class FakeGatewayTransport(MxGatewayClientOptions options) : IMx { _activeAlarmSnapshots.Add(snapshot); } + + /// + /// Maps a queued exception the way the production gRPC transport does when + /// is set; otherwise returns it unchanged. + /// + private Exception Translate(Exception exception, CallOptions callOptions) + { + if (MapTransportExceptions && exception is RpcException rpcException) + { + return RpcExceptionMapper.Map(rpcException, callOptions.CancellationToken); + } + + return exception; + } } diff --git a/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs b/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs index 7764bc6..3afa614 100644 --- a/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs +++ b/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs @@ -231,6 +231,52 @@ public sealed class MxGatewayClientSessionTests Assert.Equal("session-fixture", call.Request.SessionId); } + /// + /// Verifies that disposing a session while other callers are concurrently inside + /// — one holding the close lock and one + /// parked on it — never throws into those + /// callers. The close lock must outlive every pending close. + /// + [Fact] + public async Task DisposeAsync_DoesNotRaceConcurrentCloseAsync() + { + for (int iteration = 0; iteration < 100; iteration++) + { + FakeGatewayTransport transport = CreateTransport(); + using SemaphoreSlim firstCloseEntered = new(0, 1); + using SemaphoreSlim releaseFirstClose = new(0, 1); + + // The first CloseAsync to reach the transport parks here while holding the + // session's close lock; later callers queue on the lock behind it. + transport.CloseSessionHook = async () => + { + firstCloseEntered.Release(); + await releaseFirstClose.WaitAsync().ConfigureAwait(false); + transport.CloseSessionHook = null; + }; + + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + // Holder enters CloseAsync, acquires the lock, and parks in the hook. + Task holder = Task.Run(() => session.CloseAsync()); + await firstCloseEntered.WaitAsync(); + + // Waiter is parked on the close lock behind the holder. + Task waiter = Task.Run(() => session.CloseAsync()); + + // DisposeAsync runs concurrently; it must wait out both callers before + // disposing the close lock rather than tearing it down underneath them. + Task dispose = session.DisposeAsync().AsTask(); + + releaseFirstClose.Release(); + + await holder; + await waiter; + await dispose; + } + } + /// Verifies that invoke retries safe diagnostic commands on transient RPC failure. [Fact] public async Task InvokeAsync_RetriesSafeDiagnosticCommandOnTransientGrpcFailure() @@ -255,6 +301,35 @@ public sealed class MxGatewayClientSessionTests Assert.Equal(2, transport.InvokeCalls.Count); } + /// + /// Verifies that the retry pipeline still retries when the transport maps the raw + /// to an before it reaches + /// the retry predicate — the wrapped-exception shape that production always produces. + /// + [Fact] + public async Task InvokeAsync_RetriesSafeDiagnosticCommand_WhenTransportMapsRpcException() + { + FakeGatewayTransport transport = CreateTransport(); + transport.MapTransportExceptions = true; + transport.InvokeExceptions.Enqueue(CreateTransientRpcException()); + transport.AddInvokeReply(new MxCommandReply + { + SessionId = "session-fixture", + Kind = MxCommandKind.Ping, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + }); + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + await session.InvokeAsync(new MxCommandRequest + { + SessionId = session.SessionId, + Command = new MxCommand { Kind = MxCommandKind.Ping, Ping = new PingCommand() }, + }); + + Assert.Equal(2, transport.InvokeCalls.Count); + } + /// Verifies that open session does not retry on transient RPC failure. [Fact] public async Task OpenSessionAsync_DoesNotRetryTransientGrpcFailure() diff --git a/clients/dotnet/MxGateway.Client.Tests/RpcExceptionMapperTests.cs b/clients/dotnet/MxGateway.Client.Tests/RpcExceptionMapperTests.cs new file mode 100644 index 0000000..5d30524 --- /dev/null +++ b/clients/dotnet/MxGateway.Client.Tests/RpcExceptionMapperTests.cs @@ -0,0 +1,76 @@ +using Grpc.Core; + +namespace MxGateway.Client.Tests; + +/// Tests for the shared gRPC-to-native exception mapping used by the transports. +public sealed class RpcExceptionMapperTests +{ + /// Verifies that an unauthenticated status maps to the authentication exception. + [Fact] + public void Map_UnauthenticatedStatus_ProducesAuthenticationException() + { + RpcException rpc = new(new Status(StatusCode.Unauthenticated, "no key")); + + Exception mapped = RpcExceptionMapper.Map(rpc, CancellationToken.None); + + MxGatewayAuthenticationException authentication = + Assert.IsType(mapped); + Assert.Equal(StatusCode.Unauthenticated, authentication.StatusCode); + } + + /// Verifies that a permission-denied status maps to the authorization exception. + [Fact] + public void Map_PermissionDeniedStatus_ProducesAuthorizationException() + { + RpcException rpc = new(new Status(StatusCode.PermissionDenied, "missing scope")); + + Exception mapped = RpcExceptionMapper.Map(rpc, CancellationToken.None); + + MxGatewayAuthorizationException authorization = + Assert.IsType(mapped); + Assert.Equal(StatusCode.PermissionDenied, authorization.StatusCode); + } + + /// Verifies that a cancelled status maps to OperationCanceledException. + [Fact] + public void Map_CancelledStatus_ProducesOperationCanceledException() + { + RpcException rpc = new(new Status(StatusCode.Cancelled, "cancelled")); + + Exception mapped = RpcExceptionMapper.Map(rpc, CancellationToken.None); + + Assert.IsType(mapped); + } + + /// + /// Verifies that non-auth statuses surface the originating gRPC status code on the + /// mapped exception so callers can distinguish transient from permanent failures + /// without reflecting into InnerException. + /// + [Theory] + [InlineData(StatusCode.NotFound)] + [InlineData(StatusCode.InvalidArgument)] + [InlineData(StatusCode.ResourceExhausted)] + [InlineData(StatusCode.FailedPrecondition)] + [InlineData(StatusCode.Unavailable)] + [InlineData(StatusCode.Internal)] + public void Map_NonAuthStatus_CarriesStatusCodeOnMxGatewayException(StatusCode statusCode) + { + RpcException rpc = new(new Status(statusCode, "boom")); + + Exception mapped = RpcExceptionMapper.Map(rpc, CancellationToken.None); + + MxGatewayException gatewayException = Assert.IsType(mapped); + Assert.Equal(statusCode, gatewayException.StatusCode); + Assert.Same(rpc, gatewayException.InnerException); + } + + /// Verifies that an MxGatewayException built without a gRPC status reports a null StatusCode. + [Fact] + public void StatusCode_IsNull_WhenNoGrpcStatusProvided() + { + MxGatewayException gatewayException = new("plain failure"); + + Assert.Null(gatewayException.StatusCode); + } +} diff --git a/clients/dotnet/MxGateway.Client/GrpcGalaxyRepositoryClientTransport.cs b/clients/dotnet/MxGateway.Client/GrpcGalaxyRepositoryClientTransport.cs index e6fa05d..0b2738c 100644 --- a/clients/dotnet/MxGateway.Client/GrpcGalaxyRepositoryClientTransport.cs +++ b/clients/dotnet/MxGateway.Client/GrpcGalaxyRepositoryClientTransport.cs @@ -36,7 +36,7 @@ internal sealed class GrpcGalaxyRepositoryClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, callOptions.CancellationToken); + throw RpcExceptionMapper.Map(exception, callOptions.CancellationToken); } } @@ -53,7 +53,7 @@ internal sealed class GrpcGalaxyRepositoryClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, callOptions.CancellationToken); + throw RpcExceptionMapper.Map(exception, callOptions.CancellationToken); } } @@ -70,7 +70,7 @@ internal sealed class GrpcGalaxyRepositoryClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, callOptions.CancellationToken); + throw RpcExceptionMapper.Map(exception, callOptions.CancellationToken); } } @@ -101,7 +101,7 @@ internal sealed class GrpcGalaxyRepositoryClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, effectiveCancellationToken); + throw RpcExceptionMapper.Map(exception, effectiveCancellationToken); } yield return deployEvent; @@ -115,28 +115,4 @@ internal sealed class GrpcGalaxyRepositoryClientTransport( { return WatchDeployEventsAsync(request, callOptions); } - - private static Exception MapRpcException( - RpcException exception, - CancellationToken cancellationToken) - { - if (cancellationToken.IsCancellationRequested || exception.StatusCode == StatusCode.Cancelled) - { - return new OperationCanceledException( - exception.Status.Detail, - exception, - cancellationToken); - } - - return exception.StatusCode switch - { - StatusCode.Unauthenticated => new MxGatewayAuthenticationException( - exception.Status.Detail, - innerException: exception), - StatusCode.PermissionDenied => new MxGatewayAuthorizationException( - exception.Status.Detail, - innerException: exception), - _ => new MxGatewayException(exception.Status.Detail, exception), - }; - } } diff --git a/clients/dotnet/MxGateway.Client/GrpcMxGatewayClientTransport.cs b/clients/dotnet/MxGateway.Client/GrpcMxGatewayClientTransport.cs index 3e66b15..ef967da 100644 --- a/clients/dotnet/MxGateway.Client/GrpcMxGatewayClientTransport.cs +++ b/clients/dotnet/MxGateway.Client/GrpcMxGatewayClientTransport.cs @@ -36,7 +36,7 @@ internal sealed class GrpcMxGatewayClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, callOptions.CancellationToken); + throw RpcExceptionMapper.Map(exception, callOptions.CancellationToken); } } @@ -53,7 +53,7 @@ internal sealed class GrpcMxGatewayClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, callOptions.CancellationToken); + throw RpcExceptionMapper.Map(exception, callOptions.CancellationToken); } } @@ -70,7 +70,7 @@ internal sealed class GrpcMxGatewayClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, callOptions.CancellationToken); + throw RpcExceptionMapper.Map(exception, callOptions.CancellationToken); } } @@ -101,7 +101,7 @@ internal sealed class GrpcMxGatewayClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, effectiveCancellationToken); + throw RpcExceptionMapper.Map(exception, effectiveCancellationToken); } yield return gatewayEvent; @@ -129,7 +129,7 @@ internal sealed class GrpcMxGatewayClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, callOptions.CancellationToken); + throw RpcExceptionMapper.Map(exception, callOptions.CancellationToken); } } @@ -160,7 +160,7 @@ internal sealed class GrpcMxGatewayClientTransport( } catch (RpcException exception) { - throw MapRpcException(exception, effectiveCancellationToken); + throw RpcExceptionMapper.Map(exception, effectiveCancellationToken); } yield return snapshot; @@ -174,28 +174,4 @@ internal sealed class GrpcMxGatewayClientTransport( { return QueryActiveAlarmsAsync(request, callOptions); } - - private static Exception MapRpcException( - RpcException exception, - CancellationToken cancellationToken) - { - if (cancellationToken.IsCancellationRequested || exception.StatusCode == StatusCode.Cancelled) - { - return new OperationCanceledException( - exception.Status.Detail, - exception, - cancellationToken); - } - - return exception.StatusCode switch - { - StatusCode.Unauthenticated => new MxGatewayAuthenticationException( - exception.Status.Detail, - innerException: exception), - StatusCode.PermissionDenied => new MxGatewayAuthorizationException( - exception.Status.Detail, - innerException: exception), - _ => new MxGatewayException(exception.Status.Detail, exception), - }; - } } diff --git a/clients/dotnet/MxGateway.Client/MxGatewayAuthenticationException.cs b/clients/dotnet/MxGateway.Client/MxGatewayAuthenticationException.cs index e70b8df..a9a4bdb 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewayAuthenticationException.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewayAuthenticationException.cs @@ -1,3 +1,4 @@ +using Grpc.Core; using MxGateway.Contracts.Proto; namespace MxGateway.Client; @@ -13,6 +14,7 @@ public sealed class MxGatewayAuthenticationException : MxGatewayException /// The HResult code, if available. /// The MXAccess statuses, if available. /// The underlying exception, if any. + /// The gRPC status code reported by the failed call, if available. public MxGatewayAuthenticationException( string message, string? sessionId = null, @@ -20,7 +22,8 @@ public sealed class MxGatewayAuthenticationException : MxGatewayException ProtocolStatus? protocolStatus = null, int? hResult = null, IReadOnlyList? statuses = null, - Exception? innerException = null) + Exception? innerException = null, + StatusCode? statusCode = null) : base( message, sessionId, @@ -28,7 +31,8 @@ public sealed class MxGatewayAuthenticationException : MxGatewayException protocolStatus, hResult, statuses ?? [], - innerException) + innerException, + statusCode) { } } diff --git a/clients/dotnet/MxGateway.Client/MxGatewayAuthorizationException.cs b/clients/dotnet/MxGateway.Client/MxGatewayAuthorizationException.cs index 2df383b..1d36016 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewayAuthorizationException.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewayAuthorizationException.cs @@ -1,3 +1,4 @@ +using Grpc.Core; using MxGateway.Contracts.Proto; namespace MxGateway.Client; @@ -13,6 +14,7 @@ public sealed class MxGatewayAuthorizationException : MxGatewayException /// The HResult code, if available. /// The MXAccess statuses, if available. /// The underlying exception, if any. + /// The gRPC status code reported by the failed call, if available. public MxGatewayAuthorizationException( string message, string? sessionId = null, @@ -20,7 +22,8 @@ public sealed class MxGatewayAuthorizationException : MxGatewayException ProtocolStatus? protocolStatus = null, int? hResult = null, IReadOnlyList? statuses = null, - Exception? innerException = null) + Exception? innerException = null, + StatusCode? statusCode = null) : base( message, sessionId, @@ -28,7 +31,8 @@ public sealed class MxGatewayAuthorizationException : MxGatewayException protocolStatus, hResult, statuses ?? [], - innerException) + innerException, + statusCode) { } } diff --git a/clients/dotnet/MxGateway.Client/MxGatewayClient.cs b/clients/dotnet/MxGateway.Client/MxGatewayClient.cs index 88bbdd1..5660b8b 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewayClient.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewayClient.cs @@ -17,7 +17,7 @@ public sealed class MxGatewayClient : IAsyncDisposable private readonly GrpcChannel _channel; private readonly IMxGatewayClientTransport _transport; private readonly ResiliencePipeline _safeUnaryRetryPipeline; - private bool _disposed; + private int _disposed; /// /// Initializes a new instance of the with given options and transport. @@ -229,12 +229,11 @@ public sealed class MxGatewayClient : IAsyncDisposable /// public ValueTask DisposeAsync() { - if (_disposed) + if (Interlocked.Exchange(ref _disposed, 1) != 0) { return ValueTask.CompletedTask; } - _disposed = true; _channel?.Dispose(); return ValueTask.CompletedTask; } @@ -335,6 +334,6 @@ public sealed class MxGatewayClient : IAsyncDisposable private void ThrowIfDisposed() { - ObjectDisposedException.ThrowIf(_disposed, this); + ObjectDisposedException.ThrowIf(Volatile.Read(ref _disposed) != 0, this); } } diff --git a/clients/dotnet/MxGateway.Client/MxGatewayException.cs b/clients/dotnet/MxGateway.Client/MxGatewayException.cs index 7607317..52bdfd3 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewayException.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewayException.cs @@ -1,3 +1,4 @@ +using Grpc.Core; using MxGateway.Contracts.Proto; namespace MxGateway.Client; @@ -28,6 +29,20 @@ public class MxGatewayException : Exception Statuses = []; } + /// + /// Initializes a new instance of the MxGatewayException class carrying the originating + /// gRPC status code so callers can distinguish transient from permanent failures. + /// + /// Diagnostic message describing the failure. + /// The gRPC status code reported by the failed call. + /// Underlying exception that caused this failure. + public MxGatewayException(string message, StatusCode statusCode, Exception? innerException) + : base(message, innerException) + { + StatusCode = statusCode; + Statuses = []; + } + /// /// Initializes a new instance of the MxGatewayException class with full diagnostic information. /// @@ -38,6 +53,7 @@ public class MxGatewayException : Exception /// HRESULT code returned by the worker or MXAccess, if available. /// List of MXAccess status codes returned by the operation. /// Underlying exception that caused this failure. + /// The gRPC status code reported by the failed call, if available. public MxGatewayException( string message, string? sessionId, @@ -45,7 +61,8 @@ public class MxGatewayException : Exception ProtocolStatus? protocolStatus, int? hResult, IReadOnlyList statuses, - Exception? innerException = null) + Exception? innerException = null, + StatusCode? statusCode = null) : base(message, innerException) { SessionId = sessionId; @@ -53,6 +70,7 @@ public class MxGatewayException : Exception ProtocolStatus = protocolStatus; HResultCode = hResult; Statuses = statuses; + StatusCode = statusCode; } /// @@ -79,4 +97,15 @@ public class MxGatewayException : Exception /// Gets the list of MXAccess status codes returned by the operation. /// public IReadOnlyList Statuses { get; } + + /// + /// Gets the gRPC status code reported by the failed call, if the failure originated + /// from a gRPC . when the exception + /// was not produced from a gRPC status (for example, a protocol-level reply failure). + /// Callers can inspect this to distinguish a transient outage + /// () from a permanent error + /// () without downcasting + /// . + /// + public StatusCode? StatusCode { get; } } diff --git a/clients/dotnet/MxGateway.Client/MxGatewaySession.cs b/clients/dotnet/MxGateway.Client/MxGatewaySession.cs index 62a9a1a..ffc023e 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewaySession.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewaySession.cs @@ -9,7 +9,10 @@ public sealed class MxGatewaySession : IAsyncDisposable { private readonly MxGatewayClient _client; private readonly SemaphoreSlim _closeLock = new(1, 1); + private readonly object _disposeGate = new(); private CloseSessionReply? _closeReply; + private int _activeCloseCount; + private bool _closeLockDisposed; /// /// Initializes a new session backed by the given MXAccess gateway client. @@ -46,23 +49,42 @@ public sealed class MxGatewaySession : IAsyncDisposable return _closeReply; } - await _closeLock.WaitAsync(cancellationToken).ConfigureAwait(false); + // Register as an in-flight closer under the dispose gate. DisposeAsync waits for + // _activeCloseCount to drain before disposing the close lock, so the semaphore is + // guaranteed to outlive every WaitAsync started here. + lock (_disposeGate) + { + ObjectDisposedException.ThrowIf(_closeLockDisposed, this); + _activeCloseCount++; + } + try { - if (_closeReply is not null) + await _closeLock.WaitAsync(cancellationToken).ConfigureAwait(false); + try { + if (_closeReply is not null) + { + return _closeReply; + } + + _closeReply = await _client.CloseSessionRawAsync( + new CloseSessionRequest { SessionId = SessionId }, + cancellationToken) + .ConfigureAwait(false); return _closeReply; } - - _closeReply = await _client.CloseSessionRawAsync( - new CloseSessionRequest { SessionId = SessionId }, - cancellationToken) - .ConfigureAwait(false); - return _closeReply; + finally + { + _closeLock.Release(); + } } finally { - _closeLock.Release(); + lock (_disposeGate) + { + _activeCloseCount--; + } } } @@ -658,7 +680,32 @@ public sealed class MxGatewaySession : IAsyncDisposable /// public async ValueTask DisposeAsync() { + lock (_disposeGate) + { + if (_closeLockDisposed) + { + return; + } + } + await CloseAsync().ConfigureAwait(false); + + // Wait for every concurrent CloseAsync caller to leave the close lock before + // disposing it; once _closeReply is set those callers return without awaiting. + while (true) + { + lock (_disposeGate) + { + if (_activeCloseCount == 0) + { + _closeLockDisposed = true; + break; + } + } + + await Task.Yield(); + } + _closeLock.Dispose(); } diff --git a/clients/dotnet/MxGateway.Client/RpcExceptionMapper.cs b/clients/dotnet/MxGateway.Client/RpcExceptionMapper.cs new file mode 100644 index 0000000..9d3f6f0 --- /dev/null +++ b/clients/dotnet/MxGateway.Client/RpcExceptionMapper.cs @@ -0,0 +1,55 @@ +using Grpc.Core; + +namespace MxGateway.Client; + +/// +/// Maps low-level s raised by the gRPC stack to the client's +/// native exception hierarchy. Shared by every gateway and Galaxy Repository transport +/// so the gRPC-to-native translation has exactly one implementation. +/// +internal static class RpcExceptionMapper +{ + /// + /// Translates a into the most specific native exception type. + /// + /// The gRPC exception to translate. + /// + /// The cancellation token of the originating call; used to distinguish a caller-driven + /// cancellation from a server-side status. + /// + /// + /// An when the call was cancelled, a typed + /// authentication/authorization exception for auth statuses, or an + /// carrying the originating gRPC . + /// + public static Exception Map( + RpcException exception, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(exception); + + if (cancellationToken.IsCancellationRequested || exception.StatusCode == StatusCode.Cancelled) + { + return new OperationCanceledException( + exception.Status.Detail, + exception, + cancellationToken); + } + + return exception.StatusCode switch + { + StatusCode.Unauthenticated => new MxGatewayAuthenticationException( + exception.Status.Detail, + statusCode: exception.StatusCode, + innerException: exception), + StatusCode.PermissionDenied => new MxGatewayAuthorizationException( + exception.Status.Detail, + statusCode: exception.StatusCode, + innerException: exception), + _ => new MxGatewayException( + exception.Status.Detail, + exception.StatusCode, + exception), + }; + } +} diff --git a/clients/dotnet/README.md b/clients/dotnet/README.md index 98ff31e..7cb9977 100644 --- a/clients/dotnet/README.md +++ b/clients/dotnet/README.md @@ -112,6 +112,17 @@ can keep the full `MxCommandReply`, HRESULT, and status array when MXAccess itself rejects a command. `MxAccessException.Reply` contains the raw generated reply. +When a gRPC call itself fails, the transport maps the underlying +`RpcException` to a native exception: `Unauthenticated` becomes +`MxGatewayAuthenticationException`, `PermissionDenied` becomes +`MxGatewayAuthorizationException`, a cancelled call becomes +`OperationCanceledException`, and every other status becomes a base +`MxGatewayException`. `MxGatewayException.StatusCode` carries the originating +gRPC `Grpc.Core.StatusCode` (non-null whenever the failure came from a gRPC +status), so callers can distinguish a transient outage (`Unavailable`) from a +permanent error (`InvalidArgument`, `NotFound`) without downcasting +`InnerException`. + ## CLI Usage The test CLI supports deterministic JSON output for automation: diff --git a/code-reviews/Client.Dotnet/findings.md b/code-reviews/Client.Dotnet/findings.md index b2ad090..b2ba5fe 100644 --- a/code-reviews/Client.Dotnet/findings.md +++ b/code-reviews/Client.Dotnet/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-18 | | Commit reviewed | `3cc53a8` | | Status | Reviewed | -| Open findings | 8 | +| Open findings | 5 | ## Checklist coverage @@ -33,13 +33,13 @@ | Severity | Medium | | Category | Error handling & resilience | | Location | `clients/dotnet/MxGateway.Client/GrpcMxGatewayClientTransport.cs:190-199`, `clients/dotnet/MxGateway.Client/GrpcGalaxyRepositoryClientTransport.cs:131-140` | -| Status | Open | +| Status | Resolved | **Description:** `MapRpcException` only produces typed exceptions for `Unauthenticated` and `PermissionDenied`. Every other gRPC status — `NotFound`, `InvalidArgument`, `ResourceExhausted`, `FailedPrecondition`, `Unavailable`, `Internal` — collapses into the base `MxGatewayException` with no surfaced `StatusCode`. Callers cannot programmatically distinguish a transient outage from a permanent bad-argument error without reflecting into `InnerException` and downcasting to `RpcException`. **Recommendation:** Carry the gRPC `StatusCode` on `MxGatewayException` (e.g. a `StatusCode` property) and/or add typed subclasses for at least `NotFound`, `InvalidArgument`, and `Unavailable`. Populate it from `exception.StatusCode` in `MapRpcException`. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: both transports had a duplicated private `MapRpcException` that only typed two statuses and discarded the gRPC code for the rest. Added a nullable `StatusCode` property (`Grpc.Core.StatusCode?`) to `MxGatewayException` plus constructors that carry it, threaded it through `MxGatewayAuthenticationException`/`MxGatewayAuthorizationException`, and extracted the two duplicated mappers into a single shared internal `RpcExceptionMapper` (`RpcExceptionMapper.cs`) that populates `StatusCode` from `exception.StatusCode` for every status. Callers can now distinguish transient from permanent failures without downcasting `InnerException`. Documented in `clients/dotnet/README.md`. Regression test: `RpcExceptionMapperTests` (8 cases incl. the `[Theory]` over `NotFound`/`InvalidArgument`/`ResourceExhausted`/`FailedPrecondition`/`Unavailable`/`Internal`). ### Client.Dotnet-002 @@ -48,13 +48,13 @@ | Severity | Medium | | Category | Testing coverage | | Location | `clients/dotnet/MxGateway.Client.Tests/FakeGatewayTransport.cs:145-148`, `clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs:236-256` | -| Status | Open | +| Status | Resolved | **Description:** The retry predicate `MxGatewayClientRetryPolicy.IsTransientGrpcFailure` handles two shapes: a raw `RpcException` and an `MxGatewayException { InnerException: RpcException }`. In production the transport always maps `RpcException` → `MxGatewayException` before it reaches the retry pipeline, so only the wrapped-`MxGatewayException` branch ever runs in production. But `FakeGatewayTransport` throws the raw `RpcException` and never maps it, so every retry test exercises only the raw-`RpcException` branch — the branch that never occurs in production. The production retry behaviour is effectively untested. **Recommendation:** Add a fake/transport mode that maps `RpcException` to `MxGatewayException` the way `GrpcMxGatewayClientTransport` does (or add tests that enqueue a pre-wrapped `MxGatewayException`), so the actually-used predicate branch is covered. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: `FakeGatewayTransport` threw queued exceptions verbatim, so the existing retry tests only ever hit the raw-`RpcException` predicate branch. Added a `MapTransportExceptions` flag to `FakeGatewayTransport` that, when set, runs thrown `RpcException`s through the same shared `RpcExceptionMapper` the production gRPC transport uses, producing the wrapped `MxGatewayException` shape. Added regression test `MxGatewayClientSessionTests.InvokeAsync_RetriesSafeDiagnosticCommand_WhenTransportMapsRpcException`, which exercises the previously-untested production predicate branch. Verified red: removing the `MxGatewayException { InnerException: RpcException }` case from `IsTransientGrpcFailure` fails the new test while the pre-existing raw-`RpcException` test still passes. ### Client.Dotnet-003 @@ -63,13 +63,13 @@ | Severity | Medium | | Category | Concurrency & thread safety | | Location | `clients/dotnet/MxGateway.Client/MxGatewaySession.cs:659-663`, `clients/dotnet/MxGateway.Client/MxGatewayClient.cs:230-240` | -| Status | Open | +| Status | Resolved | **Description:** `DisposeAsync` calls `CloseAsync()` (no token) then unconditionally `_closeLock.Dispose()`. If another thread is concurrently awaiting `CloseAsync(token)` — legal, since the type exposes public async methods and no single-threaded contract — disposing the `SemaphoreSlim` while a `WaitAsync` is pending throws `ObjectDisposedException` into that caller. The `_disposed` flags in both clients are also plain unsynchronised `bool` reads/writes; `ThrowIfDisposed` racing `DisposeAsync` can observe a stale value. **Recommendation:** Either document `MxGatewaySession`/`MxGatewayClient` as not thread-safe for concurrent dispose, or guard `_disposed` with `Interlocked`/`volatile` and avoid disposing `_closeLock` until all in-flight `CloseAsync` calls complete. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: `MxGatewaySession.DisposeAsync` disposed `_closeLock` unconditionally, racing concurrent `CloseAsync` callers; `MxGatewayClient._disposed` was a plain `bool`. Fixed `MxGatewaySession` by tracking in-flight `CloseAsync` callers with an `_activeCloseCount` guarded by a dedicated `_disposeGate` lock and a `_closeLockDisposed` flag: `CloseAsync` registers under the gate (and throws `ObjectDisposedException` if disposal already won) before awaiting `_closeLock.WaitAsync`, and `DisposeAsync` drains `_activeCloseCount` to zero before disposing the semaphore, so the close lock provably outlives every pending `WaitAsync`. Fixed `MxGatewayClient` by changing `_disposed` to an `int` accessed via `Interlocked.Exchange`/`Volatile.Read`. Regression test `MxGatewayClientSessionTests.DisposeAsync_DoesNotRaceConcurrentCloseAsync` runs 100 iterations with one close holding the lock and one parked behind it while `DisposeAsync` runs concurrently; verified red against the original `DisposeAsync` (fails with `ObjectDisposedException`), green after the fix. ### Client.Dotnet-004