milestone 16: behavioral fixes for silent failures, locks, counters and the query log

src/server/doh_server.zig

@@ -63,10 +63,11 @@ const allow_header: http.Header = .{ .name = "allow", .value = "GET, POST" };
 
 pub const Options = struct {
     max_connections: u16 = default_max_connections,
-    /// The TLS handshake budget. Requests themselves have no timeout, exactly
-    /// like the web listener: the port is LAN-facing and the cancel path is
-    /// what bounds shutdown. Only the handshake — which happens before the
-    /// connection has proven it speaks anything at all — is raced.
+    /// The budget for the TLS handshake and for the wait on the next request
+    /// head of a keep-alive connection (milestone-16 ruling 10). A request
+    /// already being served has no timeout, like the web listener: the port is
+    /// LAN-facing and the cancel path is what bounds shutdown. What is raced is
+    /// every wait on a peer that owes nxdns bytes and has sent none.
     idle_timeout: std.Io.Clock.Duration = .{ .raw = .fromSeconds(10), .clock = .awake },
 };
 
@@ -76,6 +77,10 @@ pub const Stats = struct {
     rejected_at_shutdown: std.atomic.Value(u64) = .init(0),
     accept_errors: std.atomic.Value(u64) = .init(0),
     tls_handshake_failures: std.atomic.Value(u64) = .init(0),
+    /// Keep-alive connections reclaimed after `idle_timeout` elapsed with no
+    /// request head on the wire. A stalled handshake counts as a handshake
+    /// failure instead (milestone-16 ruling 9), so this name means only what
+    /// it says.
     idle_timeouts: std.atomic.Value(u64) = .init(0),
     connection_errors: std.atomic.Value(u64) = .init(0),
     /// Every 4xx answered on `/dns-query` and every miss beside it: the
@@ -308,16 +313,15 @@ pub const DohServer = struct {
             // handshake has in fact succeeded. The flag is written before the
             // race joins its tasks, so a TLS context that exists is closed on
             // every path, exactly once.
-            .timed_out => {
-                if (handshook) conn.tls.close(self.gpa);
-                bump(&self.stats.idle_timeouts);
-                return;
-            },
             .canceled => {
                 if (handshook) conn.tls.close(self.gpa);
                 return;
             },
-            .failed => {
+            // Milestone-16 ruling 9: a stalled handshake is refused like a broken
+            // one, the DoT arrangement. `idle_timeouts` belongs to the keep-alive
+            // wait below, so the two listeners export the same names for the
+            // same events.
+            .timed_out, .failed => {
                 if (handshook) conn.tls.close(self.gpa);
                 bump(&self.stats.tls_handshake_failures);
                 return;
@@ -330,11 +334,26 @@ pub const DohServer = struct {
         var connection: http.Server = .init(conn.tls.reader(), conn.tls.writer());
 
         while (connection.reader.state == .ready) {
-            var request = connection.receiveHead() catch |err| switch (err) {
+            // Milestone-16 ruling 10: the wait for the next request head is the
+            // one place a vanished keep-alive peer could pin a slot forever, so
+            // it runs under the same budget as the handshake. The body read and
+            // `handleRequest` below stay untimed.
+            var head: ReceiveHeadResult = error.ReadFailed;
+            switch (race(io, self.options.idle_timeout, receiveHeadInto, .{ &connection, &head })) {
+                .ok => {},
+                .timed_out => {
+                    bump(&self.stats.idle_timeouts);
+                    return;
+                },
+                // Cancellation is shutdown; `.failed` here is only the wrapper
+                // failing to start, which costs nothing and counts as nothing.
+                .canceled, .failed => return,
+            }
+
+            var request = head catch |err| switch (err) {
                 // The normal end of a keep-alive connection.
                 error.HttpConnectionClosing => return,
-                // Cancellation and a vanished client both land here; neither is
-                // worth a counter.
+                // A vanished client lands here; not worth a counter.
                 error.ReadFailed => return,
                 error.HttpHeadersOversize,
                 error.HttpRequestTruncated,
@@ -573,6 +592,15 @@ fn handshake(
     handshook.* = true;
 }
 
+const ReceiveHeadResult = http.Server.ReceiveHeadError!http.Server.Request;
+
+/// The DoT out-param precedent (`readPrefix`'s `out_len`): `race` needs an
+/// `anyerror!void` operation, so the request — or the error that replaced it —
+/// travels through a pointer instead of a return value.
+fn receiveHeadInto(connection: *http.Server, out: *ReceiveHeadResult) anyerror!void {
+    out.* = connection.receiveHead();
+}
+
 /// True when the head frames body bytes on the wire. A bare
 /// `content-length: 0` frames nothing.
 fn framesBody(transfer_encoding: http.TransferEncoding, content_length: ?u64) bool {
@@ -878,6 +906,10 @@ const Harness = struct {
     group: std.Io.Group,
 
     fn start(hx: *Harness) !void {
+        return hx.startWith(.{ .max_connections = 4 });
+    }
+
+    fn startWith(hx: *Harness, options: Options) !void {
         hx.threaded = .init(testing.allocator, .{});
         errdefer hx.threaded.deinit();
         const hio = hx.threaded.io();
@@ -904,9 +936,7 @@ const Harness = struct {
         };
 
         const listen_address: net.IpAddress = try .parse("127.0.0.1", 0);
-        hx.server = try DohServer.listen(testing.allocator, hio, listen_address, &hx.h, &hx.store, .{
-            .max_connections = 4,
-        });
+        hx.server = try DohServer.listen(testing.allocator, hio, listen_address, &hx.h, &hx.store, options);
         errdefer hx.server.deinit(testing.allocator, hio);
 
         hx.group = .init;
@@ -1462,6 +1492,42 @@ fn twoPostsOneConnection(io: std.Io, remote: net.IpAddress) anyerror!void {
     try conn.end();
 }
 
+/// Long enough for a loopback round trip, short enough that the `bounded`
+/// budget still fails a server that never reclaims the connection.
+const short_idle: std.Io.Clock.Duration = .{ .raw = .fromMilliseconds(300), .clock = .awake };
+
+/// One request, then silence on a connection the client keeps open. The server
+/// owes no answer, so the only thing that can end the read is the idle budget.
+fn idleAfterOneRequest(io: std.Io, remote: net.IpAddress) anyerror!void {
+    var conn: ClientConn = undefined;
+    try conn.connect(io, remote);
+    defer conn.close(io);
+
+    try sendPost(&conn, doh_client.media_type, query_bytes);
+    const resp = try readResponse(&conn.client.reader);
+    try testing.expectEqual(@as(u16, 200), resp.status);
+    try expectLocalReply(resp.body());
+
+    try expectEof(&conn.client.reader);
+}
+
+test "doh: an idle keep-alive connection is reclaimed and counted" {
+    if (!build_options.integration) return error.SkipZigTest;
+
+    var hx: Harness = undefined;
+    try hx.startWith(.{ .max_connections = 4, .idle_timeout = short_idle });
+    defer hx.stop();
+
+    try bounded(hx.io(), idleAfterOneRequest, .{ hx.io(), hx.addr() });
+
+    const stats = hx.server.snapshotStats();
+    try testing.expectEqual(@as(u64, 1), stats.connections);
+    try testing.expectEqual(@as(u64, 1), stats.idle_timeouts);
+    try testing.expectEqual(@as(u64, 0), stats.tls_handshake_failures);
+    try testing.expectEqual(@as(u64, 0), stats.connection_errors);
+    try testing.expectEqual(@as(u64, 0), stats.bad_requests);
+}
+
 test "keep-alive: two requests are answered on one connection" {
     if (!build_options.integration) return error.SkipZigTest;