fix(server): keep unmarkable listeners serving

crates/irtt-server/src/runtime.rs

@@ -44,6 +44,14 @@ pub struct Server {
     /// queried at construction, in which case the send path asks again rather
     /// than guessing a family.
     listener_is_ipv4: Option<bool>,
+    /// Whether this server has ever successfully applied a nonzero traffic
+    /// class to the socket.
+    ///
+    /// This is not a cache and never elides a call — the class is applied
+    /// before every send regardless. It is consulted only when applying one
+    /// *fails*, to tell "the socket may be carrying a marking of ours" from
+    /// "it cannot be carrying one".
+    socket_is_marked: bool,
 }
 
 impl Server {
@@ -65,16 +73,36 @@ impl Server {
             core: ServerCore::new(config),
             recv_buffer: vec![0; RECEIVE_BUFFER_LEN],
             listener_is_ipv4,
+            socket_is_marked: false,
         }
     }
 
-    /// Applies one reply's raw traffic class to the listener socket.
+    /// Applies one reply's raw traffic class to the listener socket, and
+    /// reports whether that reply may be sent.
     ///
-    /// This runs before *every* send, and zero is applied as deliberately as
+    /// The class is applied before *every* send, and zero as deliberately as
     /// any other value: a listener serves many sessions from one socket, so
     /// skipping the call for an unmarked reply — an open reply, or a session
     /// that negotiated nothing — would send it under whichever marking the
     /// previous reply left behind.
+    ///
+    /// When the option cannot be applied, [`may_send_unappliable`] decides. A
+    /// marking this server asked for and did not get must never be replaced by
+    /// silently sending under the previous one, so those replies are dropped;
+    /// but a host that refuses the option outright — some Windows builds do not
+    /// support `IP_TOS`, and a few targets have no safe setter at all — must
+    /// still be able to run a server, and it can, because a socket this server
+    /// has never marked has no marking of ours to clear.
+    fn prepare_reply_traffic_class(&mut self, traffic_class: u8) -> bool {
+        match self.apply_reply_traffic_class(traffic_class) {
+            Ok(()) => {
+                self.socket_is_marked = traffic_class != 0;
+                true
+            }
+            Err(_) => may_send_unappliable(traffic_class, self.socket_is_marked),
+        }
+    }
+
     fn apply_reply_traffic_class(&self, traffic_class: u8) -> io::Result<()> {
         let is_ipv4 = match self.listener_is_ipv4 {
             Some(is_ipv4) => is_ipv4,
@@ -95,13 +123,16 @@ impl Server {
     /// Graceful shutdown sends no session-close packets and leaves no hidden
     /// receive task behind, even while a reply send is waiting for socket
     /// writability. Per-packet send failures and short sends drop only that
-    /// reply, and so does a failure to apply that reply's traffic class —
+    /// reply, and so does a failure to apply a marking that reply needed —
     /// preparing one packet is not a reason to stop serving every other
     /// session, and the next reply makes its own independent attempt. Receive
     /// failures and internal core failures terminate the loop with an error.
     ///
-    /// A reply whose class could not be applied is never sent anyway: sending it
-    /// would put it on the wire under the previous reply's marking.
+    /// A reply whose marking could not be applied is not sent instead: sending
+    /// it would put it on the wire under the previous reply's marking. An
+    /// *unmarked* reply on a socket this server has never marked is the one
+    /// exception, so a host that does not support the option at all still runs
+    /// a working server. See [`Server::prepare_reply_traffic_class`].
     pub async fn run<F>(&mut self, shutdown: F) -> Result<(), ServerRuntimeError>
     where
         F: Future<Output = ()>,
@@ -126,7 +157,7 @@ impl Server {
                     }
 
                     if let Some(reply) = self.core.handle_datagram(peer, &self.recv_buffer[..len])? {
-                        if self.apply_reply_traffic_class(reply.traffic_class()).is_err() {
+                        if !self.prepare_reply_traffic_class(reply.traffic_class()) {
                             continue;
                         }
                         let send = self.socket.send_to(reply.bytes(), peer);
@@ -148,6 +179,21 @@ impl Server {
     }
 }
 
+/// Whether a reply may still be sent after its traffic class could not be
+/// applied.
+///
+/// Only an unmarked reply on a socket this server has never successfully marked
+/// qualifies. Both halves matter: a reply that wanted a marking must not go out
+/// without one, and once this server has put a nonzero class on the socket, a
+/// failure to clear it means the socket may still be carrying it.
+///
+/// A socket handed to [`Server::from_socket`] pre-marked by its creator is
+/// outside what this can know; explicit marking is the server's own to manage
+/// from a fresh listener.
+fn may_send_unappliable(traffic_class: u8, socket_is_marked: bool) -> bool {
+    traffic_class == 0 && !socket_is_marked
+}
+
 /// Failure to create or run a Tokio UDP server.
 #[derive(Debug, Error)]
 pub enum ServerRuntimeError {
@@ -323,4 +369,33 @@ mod tests {
     fn unthrottled() -> ServerConfig {
         ServerConfig::default().with_min_send_interval(Duration::ZERO)
     }
+
+    /// The rule that decides what an unappliable traffic class means, which no
+    /// normal interface can reach: it needs a host that refuses the socket
+    /// option, and the hosts these tests run on do not.
+    #[test]
+    fn only_an_unmarked_reply_on_a_never_marked_socket_survives_an_apply_failure() {
+        for (traffic_class, socket_is_marked, sendable, why) in [
+            (
+                0,
+                false,
+                true,
+                "no marking wanted, and none of ours to clear",
+            ),
+            (0, true, false, "our own marking may still be on the socket"),
+            (
+                0xb8,
+                false,
+                false,
+                "a marking that was wanted but not applied",
+            ),
+            (0xb8, true, false, "and likewise over a marking already set"),
+        ] {
+            assert_eq!(
+                may_send_unappliable(traffic_class, socket_is_marked),
+                sendable,
+                "{why}"
+            );
+        }
+    }
 }

crates/irtt-server/src/socket_options.rs

@@ -34,8 +34,8 @@ pub(crate) fn set_reply_traffic_class(
     target_os = "fuchsia",
     target_os = "redox",
     target_os = "solaris",
-    target_os = "illumos",
     target_os = "haiku",
+    target_os = "wasi",
 )))]
 fn set_ipv4_traffic_class(socket: SockRef<'_>, traffic_class: u32) -> io::Result<()> {
     socket.set_tos_v4(traffic_class)
@@ -45,8 +45,8 @@ fn set_ipv4_traffic_class(socket: SockRef<'_>, traffic_class: u32) -> io::Result
     target_os = "fuchsia",
     target_os = "redox",
     target_os = "solaris",
-    target_os = "illumos",
     target_os = "haiku",
+    target_os = "wasi",
 ))]
 fn set_ipv4_traffic_class(_socket: SockRef<'_>, traffic_class: u32) -> io::Result<()> {
     unsupported_traffic_class(traffic_class, "IPv4 TOS socket options")
@@ -62,6 +62,7 @@ fn set_ipv4_traffic_class(_socket: SockRef<'_>, traffic_class: u32) -> io::Resul
     target_os = "netbsd",
     target_os = "openbsd",
     target_os = "cygwin",
+    target_os = "illumos",
 ))]
 fn set_ipv6_traffic_class(socket: SockRef<'_>, traffic_class: u32) -> io::Result<()> {
     socket.set_tclass_v6(traffic_class)
@@ -77,6 +78,7 @@ fn set_ipv6_traffic_class(socket: SockRef<'_>, traffic_class: u32) -> io::Result
     target_os = "netbsd",
     target_os = "openbsd",
     target_os = "cygwin",
+    target_os = "illumos",
 )))]
 fn set_ipv6_traffic_class(_socket: SockRef<'_>, traffic_class: u32) -> io::Result<()> {
     unsupported_traffic_class(traffic_class, "IPv6 Traffic Class socket options")
@@ -84,18 +86,20 @@ fn set_ipv6_traffic_class(_socket: SockRef<'_>, traffic_class: u32) -> io::Resul
 
 /// A target where the safe API cannot express this option.
 ///
-/// Requesting *unmarked* still succeeds, because a platform that can never
-/// apply a nonzero class also never left one on the socket for the next reply to
-/// inherit — the invariant the explicit zero exists to protect. Asking for a
-/// real marking reports [`io::ErrorKind::Unsupported`] rather than pretending:
-/// the runtime drops that one reply and stays alive, and no reply ever goes out
-/// carrying a class the server did not intend.
+/// This reports [`io::ErrorKind::Unsupported`] for *every* value, zero
+/// included. Returning success for zero would claim the socket had been cleared
+/// when nothing was written to it, which is not the same statement — a caller
+/// handing [`Server::from_socket`](crate::Server::from_socket) an already
+/// prepared socket may have marked it by some other means. Whether an
+/// unappliable zero is nevertheless safe to send is the runtime's decision, and
+/// it is made from what this server itself has applied; this helper's job is
+/// only to report honestly.
 #[cfg(any(
     target_os = "fuchsia",
     target_os = "redox",
     target_os = "solaris",
-    target_os = "illumos",
     target_os = "haiku",
+    target_os = "wasi",
     not(any(
         target_os = "android",
         target_os = "dragonfly",
@@ -106,15 +110,12 @@ fn set_ipv6_traffic_class(_socket: SockRef<'_>, traffic_class: u32) -> io::Resul
         target_os = "netbsd",
         target_os = "openbsd",
         target_os = "cygwin",
+        target_os = "illumos",
     )),
 ))]
-fn unsupported_traffic_class(traffic_class: u32, option: &'static str) -> io::Result<()> {
-    if traffic_class == 0 {
-        Ok(())
-    } else {
-        Err(io::Error::new(
-            io::ErrorKind::Unsupported,
-            format!("{option} are unsupported on this target"),
-        ))
-    }
+fn unsupported_traffic_class(_traffic_class: u32, option: &'static str) -> io::Result<()> {
+    Err(io::Error::new(
+        io::ErrorKind::Unsupported,
+        format!("{option} are unsupported on this target"),
+    ))
 }