fix(host): h2 keepalive on site gRPC listener (arch-review 2026-08-19 P4-3)

2026-08-19 14:49:37 -04:00
parent e63ffd90ee
commit b68f553f1e
3 changed files with 125 additions and 14 deletions
@@ -757,21 +757,10 @@ try
        // metrics listener never collides with the Akka remoting port on site nodes.
        var metricsPort = configuration.GetValue<int>("ScadaBridge:Node:MetricsPort", 8084);

-        // Configure Kestrel for HTTP/2 only on the gRPC port
+        // Configure Kestrel for HTTP/2 only on the gRPC port (plus the h2 keepalive that
+        // makes a half-open inbound stream detectable) — see SiteKestrelSetup.
        builder.WebHost.ConfigureKestrel(options =>
-        {
-            options.ListenAnyIP(grpcPort, listenOptions =>
-            {
-                listenOptions.Protocols = Microsoft.AspNetCore.Server.Kestrel.Core.HttpProtocols.Http2;
-            });
-
-            // Dedicated HTTP/1.1 (and HTTP/2) listener for the Prometheus /metrics
-            // scrape endpoint, reachable by an HTTP/1.1 scraper.
-            options.ListenAnyIP(metricsPort, listenOptions =>
-            {
-                listenOptions.Protocols = Microsoft.AspNetCore.Server.Kestrel.Core.HttpProtocols.Http1AndHttp2;
-            });
-        });
+            SiteKestrelSetup.Configure(options, grpcPort, metricsPort));

        // gRPC server registration. Two interceptors, two disjoint service prefixes, two
        // separate keys — neither one's absence weakens the other:
@@ -0,0 +1,66 @@
+using Microsoft.AspNetCore.Server.Kestrel.Core;
+
+namespace ZB.MOM.WW.ScadaBridge.Host;
+
+/// <summary>
+/// Kestrel configuration for a Site node: the h2c gRPC listener, the HTTP/1.1 metrics listener, and
+/// the HTTP/2 keepalive that governs both.
+/// </summary>
+/// <remarks>
+/// Extracted from <c>Program.cs</c>'s Site branch so the keepalive settings are assertable — the
+/// existing Host wiring tests re-declare their own <c>ConfigureKestrel</c> lambda and therefore
+/// prove nothing about the one that ships.
+/// </remarks>
+internal static class SiteKestrelSetup
+{
+    /// <summary>
+    /// How long an HTTP/2 connection may sit idle before Kestrel sends a keepalive PING.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// Kestrel's default is <see cref="TimeSpan.MaxValue"/> — no server-initiated pings at all — and
+    /// every long-lived stream this port carries is legitimately silent for long stretches: the
+    /// LocalDb replication sync stream on a converged pair, and a <c>SiteStreamService</c>
+    /// subscription for an idle site. A hard crash or network partition of the peer therefore leaves
+    /// this node holding a half-open stream that will never produce another byte and never fail
+    /// either. For LocalDb sync that is the expensive one: the handler sits inside its single-stream
+    /// guard, so the rebooted peer's reconnects are refused while this node still reports itself
+    /// replication-connected (ScadaBridge arch-review 2026-08-19, P4-3).
+    /// </para>
+    /// <para>
+    /// 60s costs one frame per idle minute per connection and bounds detection at roughly
+    /// <see cref="Http2KeepAlivePingDelay"/> + <see cref="Http2KeepAlivePingTimeout"/>. Pings are
+    /// ordinary HTTP/2 frames, so this is unrelated to the listener being h2c rather than TLS.
+    /// </para>
+    /// </remarks>
+    internal static readonly TimeSpan Http2KeepAlivePingDelay = TimeSpan.FromSeconds(60);
+
+    /// <summary>How long Kestrel waits for a keepalive PING ack before closing the connection.</summary>
+    internal static readonly TimeSpan Http2KeepAlivePingTimeout = TimeSpan.FromSeconds(20);
+
+    /// <summary>
+    /// Binds the site node's two listeners and applies the server-wide HTTP/2 keepalive.
+    /// </summary>
+    /// <remarks>
+    /// The keepalive lives on <c>Limits</c>, which is per-server rather than per-listener, so it
+    /// governs the gRPC port and the metrics port alike. That is the intent: the metrics listener
+    /// also speaks HTTP/2, and a scrape connection has nothing to lose from a ping.
+    /// </remarks>
+    public static void Configure(KestrelServerOptions options, int grpcPort, int metricsPort)
+    {
+        options.Limits.Http2.KeepAlivePingDelay = Http2KeepAlivePingDelay;
+        options.Limits.Http2.KeepAlivePingTimeout = Http2KeepAlivePingTimeout;
+
+        options.ListenAnyIP(grpcPort, listenOptions =>
+        {
+            listenOptions.Protocols = HttpProtocols.Http2;
+        });
+
+        // Dedicated HTTP/1.1 (and HTTP/2) listener for the Prometheus /metrics
+        // scrape endpoint, reachable by an HTTP/1.1 scraper.
+        options.ListenAnyIP(metricsPort, listenOptions =>
+        {
+            listenOptions.Protocols = HttpProtocols.Http1AndHttp2;
+        });
+    }
+}
@@ -0,0 +1,56 @@
+using Microsoft.AspNetCore.Server.Kestrel.Core;
+using ZB.MOM.WW.ScadaBridge.Host;
+
+namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
+
+/// <summary>
+/// The site node's Kestrel configuration, asserted against the code that actually ships (the other
+/// Host startup tests re-declare their own <c>ConfigureKestrel</c> lambda, which can never catch a
+/// regression in <c>Program.cs</c>'s).
+/// </summary>
+public class SiteKestrelSetupTests
+{
+    [Fact]
+    public void SiteListener_EnablesHttp2Keepalive()
+    {
+        // Arch-review 2026-08-19 P4-3. Kestrel's default KeepAlivePingDelay is TimeSpan.MaxValue —
+        // no server-initiated pings — so a half-open HTTP/2 connection is never detected on a
+        // stream that is legitimately silent. The LocalDb sync stream is exactly that: with the
+        // dialling node crashed or partitioned, this node's handler stays parked on a dead stream
+        // holding the single-stream guard, refusing the reconnect that would heal the pair.
+        var options = new KestrelServerOptions();
+
+        SiteKestrelSetup.Configure(options, grpcPort: 8083, metricsPort: 8084);
+
+        Assert.NotEqual(TimeSpan.MaxValue, options.Limits.Http2.KeepAlivePingDelay);
+        Assert.Equal(SiteKestrelSetup.Http2KeepAlivePingDelay, options.Limits.Http2.KeepAlivePingDelay);
+        Assert.Equal(SiteKestrelSetup.Http2KeepAlivePingTimeout, options.Limits.Http2.KeepAlivePingTimeout);
+        // Detection has to be quick enough to matter, and a ping must not be its own timeout.
+        Assert.True(SiteKestrelSetup.Http2KeepAlivePingTimeout < SiteKestrelSetup.Http2KeepAlivePingDelay);
+        Assert.True(
+            SiteKestrelSetup.Http2KeepAlivePingDelay + SiteKestrelSetup.Http2KeepAlivePingTimeout
+                <= TimeSpan.FromMinutes(2));
+    }
+
+    [Fact]
+    public void SiteListener_KeepsGrpcHttp2Only_AndMetricsHttp1Capable()
+    {
+        // The keepalive change must not disturb the protocol split: h2c-only on the gRPC port so
+        // Grpc.AspNetCore can serve it without TLS ALPN, HTTP/1.1-capable on the metrics port so a
+        // standard Prometheus scraper can reach /metrics.
+        var options = new KestrelServerOptions();
+
+        SiteKestrelSetup.Configure(options, grpcPort: 8083, metricsPort: 8084);
+
+        var endpoints = options.GetType()
+            .GetProperty("CodeBackedListenOptions", System.Reflection.BindingFlags.Instance
+                | System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Public)!
+            .GetValue(options) as System.Collections.IEnumerable;
+        Assert.NotNull(endpoints);
+
+        var protocols = endpoints!.Cast<ListenOptions>().Select(o => o.Protocols).ToList();
+        Assert.Equal(2, protocols.Count);
+        Assert.Equal(HttpProtocols.Http2, protocols[0]);
+        Assert.Equal(HttpProtocols.Http1AndHttp2, protocols[1]);
+    }
+}