Check for sentinel value when setting HTTP/3 error code (#57934)

* Check for sentinel value when setting HTTP/3 error code

If no error code has been set, `IProtocolErrorFeature.Error` will be `-1`.  If we pass that through verbatim, it will be caught by validation in the setter (ironically, of the same property on the same feature object), resulting in an exception and a Critical (but apparently benign) log message.

Fixes #57933

* Cover a couple other consumers of IProtocolErrorCodeFeature

* Add explanatory comment

Co-authored-by: James Newton-King <[email protected]>

* Use a property

* Add a regression test

---------

Co-authored-by: James Newton-King <[email protected]>

Author: 2 people

src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs

@@ -101,6 +101,9 @@ private void UpdateHighestOpenedRequestStreamId(long streamId)
     public string ConnectionId => _context.ConnectionId;
     public ITimeoutControl TimeoutControl => _context.TimeoutControl;
 
+    // The default error value is -1. If it hasn't been changed before abort is called then default to HTTP/3's NoError value.
+    private Http3ErrorCode Http3ErrorCodeOrNoError => _errorCodeFeature.Error == -1 ? Http3ErrorCode.NoError : (Http3ErrorCode)_errorCodeFeature.Error;
+
     public void StopProcessingNextRequest(ConnectionEndReason reason)
         => StopProcessingNextRequest(serverInitiated: true, reason);
 
@@ -505,12 +508,14 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
                     }
                 }
 
+                var errorCode = Http3ErrorCodeOrNoError;
+
                 // Abort active request streams.
                 lock (_streams)
                 {
                     foreach (var stream in _streams.Values)
                     {
-                        stream.Abort(CreateConnectionAbortError(error, clientAbort), (Http3ErrorCode)_errorCodeFeature.Error);
+                        stream.Abort(CreateConnectionAbortError(error, clientAbort), errorCode);
                     }
                 }
 
@@ -546,7 +551,7 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
                 }
 
                 // Complete
-                Abort(CreateConnectionAbortError(error, clientAbort), (Http3ErrorCode)_errorCodeFeature.Error, reason);
+                Abort(CreateConnectionAbortError(error, clientAbort), errorCode, reason);
 
                 // Wait for active requests to complete.
                 while (_activeRequestCount > 0)
@@ -905,7 +910,7 @@ public void OnInputOrOutputCompleted()
         TryStopAcceptingStreams();
 
         // Abort the connection using the error code the client used. For a graceful close, this should be H3_NO_ERROR.
-        Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient), (Http3ErrorCode)_errorCodeFeature.Error, ConnectionEndReason.TransportCompleted);
+        Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient), Http3ErrorCodeOrNoError, ConnectionEndReason.TransportCompleted);
     }
 
     internal WebTransportSession OpenNewWebTransportSession(Http3Stream http3Stream)

src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs

@@ -225,7 +225,7 @@ public void TriggerTick(TimeSpan timeSpan = default)
 
     public async Task InitializeConnectionAsync(RequestDelegate application)
     {
-        MultiplexedConnectionContext = new TestMultiplexedConnectionContext(this)
+        MultiplexedConnectionContext ??= new TestMultiplexedConnectionContext(this)
         {
             ConnectionId = "TEST"
         };

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs

@@ -701,6 +701,84 @@ await Http3Api.InitializeConnectionAsync(async c =>
         await tcs.Task;
     }
 
+    [Fact]
+    public async Task ErrorCodeIsValidOnConnectionTimeout()
+    {
+        // This test loosely repros the scenario in https://github.com/dotnet/aspnetcore/issues/57933.
+        // In particular, there's a request from the server and, once a response has been sent,
+        // the (simulated) transport throws a QuicException that surfaces through AcceptAsync.
+        // This test confirms that Http3Connection.ProcessRequestsAsync doesn't (indirectly) cause
+        // IProtocolErrorCodeFeature.Error to be set to (or left at) -1, which System.Net.Quic will
+        // not accept.
+
+        // Used to signal that a request has been sent and a response has been received
+        var requestTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        // Used to signal that the connection context has been aborted
+        var abortTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+
+        // InitializeConnectionAsync consumes the connection context, so set it first
+        Http3Api.MultiplexedConnectionContext = new ThrowingMultiplexedConnectionContext(Http3Api, skipCount: 2, requestTcs, abortTcs);
+        await Http3Api.InitializeConnectionAsync(_echoApplication);
+
+        await Http3Api.CreateControlStream();
+        await Http3Api.GetInboundControlStream();
+        var requestStream = await Http3Api.CreateRequestStream(Headers, endStream: true);
+        var responseHeaders = await requestStream.ExpectHeadersAsync();
+
+        await requestStream.ExpectReceiveEndOfStream();
+        await requestStream.OnDisposedTask.DefaultTimeout();
+
+        requestTcs.SetResult();
+
+        // By the time the connection context is aborted, the error code feature has been updated
+        await abortTcs.Task.DefaultTimeout();
+
+        Http3Api.CloseServerGracefully();
+
+        var errorCodeFeature = Http3Api.MultiplexedConnectionContext.Features.Get<IProtocolErrorCodeFeature>();
+        Assert.InRange(errorCodeFeature.Error, 0, (1L << 62) - 1); // Valid range for HTTP/3 error codes
+    }
+
+    private sealed class ThrowingMultiplexedConnectionContext : TestMultiplexedConnectionContext
+    {
+        private int _skipCount;
+        private readonly TaskCompletionSource _requestTcs;
+        private readonly TaskCompletionSource _abortTcs;
+
+        /// <summary>
+        /// After <paramref name="skipCount"/> calls to <see cref="AcceptAsync"/>, the next call will throw a <see cref="QuicException"/>
+        /// (after waiting for <see cref="_requestTcs"/> to be set).
+        ///
+        /// <paramref name="abortTcs"/> lets this type signal that <see cref="Abort"/> has been called.
+        /// </summary>
+        public ThrowingMultiplexedConnectionContext(Http3InMemory testBase, int skipCount, TaskCompletionSource requestTcs, TaskCompletionSource abortTcs)
+            : base(testBase)
+        {
+            _skipCount = skipCount;
+            _requestTcs = requestTcs;
+            _abortTcs = abortTcs;
+        }
+
+        public override async ValueTask<ConnectionContext> AcceptAsync(CancellationToken cancellationToken = default)
+        {
+            if (_skipCount-- <= 0)
+            {
+                await _requestTcs.Task.DefaultTimeout();
+                throw new System.Net.Quic.QuicException(
+                    System.Net.Quic.QuicError.ConnectionTimeout,
+                    applicationErrorCode: null,
+                    "Connection timed out waiting for a response from the peer.");
+            }
+            return await base.AcceptAsync(cancellationToken);
+        }
+
+        public override void Abort(ConnectionAbortedException abortReason)
+        {
+            _abortTcs.SetResult();
+            base.Abort(abortReason);
+        }
+    }
+
     private async Task<ConnectionContext> MakeRequestAsync(int index, KeyValuePair<string, string>[] headers, bool sendData, bool waitForServerDispose)
     {
         var requestStream = await Http3Api.CreateRequestStream(headers, endStream: !sendData);