From 73429dfc0d8e8ee781beaedbeb45bce216e13e72 Mon Sep 17 00:00:00 2001 From: zzstoatzz Date: Fri, 24 Jul 2026 23:02:41 -0500 Subject: [PATCH] client: best-effort setsockopt on connection sockets std.posix.setsockopt maps BADF/NOTSOCK/INVAL/FAULT to `unreachable` ("always a race condition"). That holds for a listening socket. It does not hold for a connection socket: the peer can reset, or another thread can close the fd, between connect and the timeout call. `unreachable` cannot be caught, so the race aborts the process instead of returning an error the caller could reconnect on. Found downstream in Stream (zat.dev/stream), whose firehose consumer aborted roughly one full-suite run in seven with `reached unreachable code` in setsockopt, reached from Stream.setsockopt via readTimeout inside HandShakeReply.read during zat's connectAndRead. An upstream relay dropping the connection around handshake killed the process -- previously misread as a transient transport error. Route the client's connection-socket setsockopt through setConnSockOpt, which issues the raw syscall and swallows exactly the four arms the stdlib declares impossible while keeping every other errno meaningful. The timeouts are advisory: if the socket really is gone, the following read or write reports it properly. This is the client-side counterpart of 00bed03, which did the same for the four server connection-socket call sites. Stream.setsockopt keeps its `!void` signature so no consumer's `try` breaks. Test closes the fd under a live Stream and asserts the timeout calls return instead of aborting; before this change it panics on `.BADF => unreachable`. 64 of 64 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- src/client/client.zig | 58 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/src/client/client.zig b/src/client/client.zig index 605776c..735c976 100644 --- a/src/client/client.zig +++ b/src/client/client.zig @@ -598,10 +598,46 @@ pub const Stream = struct { } pub fn setsockopt(self: *const Stream, opt_name: u32, value: []const u8) !void { - return posix.setsockopt(self.stream.socket.handle, posix.SOL.SOCKET, opt_name, value); + return setConnSockOpt(self.stream.socket.handle, posix.SOL.SOCKET, opt_name, value); } }; +/// setsockopt for a *connection* socket. +/// +/// std.posix.setsockopt maps BADF/NOTSOCK/INVAL/FAULT to `unreachable` +/// ("always a race condition"). On a listening socket that holds. On a +/// connection socket it does not: the peer can reset, or another thread can +/// close the fd, between connect and the timeout call. `unreachable` cannot be +/// caught, so that race aborts the whole process instead of surfacing as an +/// error the caller's reconnect path would handle — observed as a panic inside +/// the client handshake when an upstream dropped the connection. +/// +/// FAULT is deliberately NOT swallowed: a bad option pointer is our own bug, +/// not a peer's doing, and hiding it would trade one silent failure for +/// another. +/// +/// The timeouts this backs are advisory: if the socket really is gone, the +/// following read or write reports it properly. So swallow exactly the arms the +/// stdlib calls impossible and keep every other error meaningful. +/// +/// The server side took the same approach in `setSockOptBestEffort` +/// (src/server/server.zig). +fn setConnSockOpt(fd: posix.socket_t, level: i32, optname: u32, opt: []const u8) !void { + switch (posix.errno(posix.system.setsockopt(fd, level, optname, opt.ptr, @intCast(opt.len)))) { + .SUCCESS => {}, + // The socket died under us. Not our failure to report. + .BADF, .NOTSOCK, .INVAL => {}, + .DOM => return error.TimeoutTooBig, + .ISCONN => return error.AlreadyConnected, + .NOPROTOOPT => return error.InvalidProtocolOption, + .NOMEM, .NOBUFS => return error.SystemResources, + .PERM => return error.PermissionDenied, + .NODEV => return error.NoDevice, + .OPNOTSUPP => return error.OperationUnsupported, + else => |err| return posix.unexpectedErrno(err), + } +} + const TLSClient = struct { client: tls.Client, stream: net.Stream, @@ -1073,6 +1109,26 @@ test "Client: handshake with terminating CRLF split across reads" { try t.expectSlice(u8, trailing, buf[0..res.over_read]); } +test "Client: setting a timeout on a dead socket does not abort the process" { + // regression: std.posix.setsockopt maps BADF/NOTSOCK/INVAL/FAULT to + // `unreachable`. Those are reachable on a *connection* socket — the peer + // resets, or another thread closes the fd, between connect and the timeout + // call. `unreachable` cannot be caught, so an upstream dropping the + // connection aborted the whole process from inside the handshake instead of + // returning an error the caller could reconnect on. + const io = std.Options.debug_io; + const stream = try testConnectedStream(io, "127.0.0.1", 9292); + var dead = Stream.init(io, stream, null); + // Close underneath the Stream: every later setsockopt sees EBADF, which is + // precisely the race the stdlib declares impossible. + stream.close(io); + + // Before the fix each of these panicked rather than returning. + try dead.readTimeout(5_000); + try dead.writeTimeout(5_000); + try dead.readTimeout(0); +} + test "Client: write/read" { const io = std.Options.debug_io; const stream = try testConnectedStream(io, "127.0.0.1", 9292); -- knotmirror 0.0.1