Fix spurious SACKs by respecting delayed ACK timer

SCTP employs a "Delayed Acknowledgement" algorithm (RFC 9260, 
section 6.2) to reduce network traffic. Instead of immediately 
acknowledging every received DATA chunk, the protocol allows the 
receiver to delay sending a Selective Acknowledgement (SACK) for up to 
200ms (or until a second packet arrives). This allows the SACK to be 
"piggybacked" onto outgoing user data or bundled with other control 
chunks, reducing the number of small packets sent.

Before this change, the `Socket::advance_time` method broke this by
unconditionally flushing pending packets. Whenever `advance_time` was
called (which the application is allowed to do at any time), it would 
trigger packet transmission logic that forced any pending SACK to be 
sent immediately. This broke the delayed SACK optimization, resulting in 
spurious, standalone SACK transmissions whenever the socket clock 
advanced (when there was a pending and delayed SACK).

This commit fixes the issue by ensuring `advance_time` only attempts to 
send packets if a timer (such as the delayed ACK timer) has actually 
expired. And it also modifies the packet builder to only include a 
pending SACK if it can be bundled with outgoing data/retransmissions, or 
if the delayed ACK timer has explicitly expired.

Author: boivie

CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to
  - Corrected "Fast Recovery" retransmission logic.
  - Corrected outstanding data calculation.
  - Added OnLifecycleMessageFullySent lifecycle events when sending message.
+ - Fixed bug where delayed SACKs would be sent immediately when time advances.
 
 ## 0.1.8 - 2026-02-06
 

src/rx/data_tracker.rs

@@ -354,10 +354,12 @@ impl DataTracker {
         self.update_ack_state(now, AckState::Immediate);
     }
 
-    pub fn handle_timeout(&mut self, now: SocketTime) {
-        if self.delayed_ack_timer.expire(now) {
-            self.update_ack_state(now, AckState::Immediate);
+    pub fn handle_timeout(&mut self, now: SocketTime) -> bool {
+        if !self.delayed_ack_timer.expire(now) {
+            return false;
         }
+        self.update_ack_state(now, AckState::Immediate);
+        true
     }
 
     /// Called at the end of processing an SCTP packet.

src/socket/connect.rs

@@ -422,26 +422,36 @@ pub(crate) fn handle_cookie_ack(state: &mut State, ctx: &mut Context, now: Socke
     ctx.events.borrow_mut().add(SocketEvent::OnConnected());
 }
 
-pub(crate) fn handle_t1init_timeout(state: &mut State, ctx: &mut Context, now: SocketTime) {
+pub(crate) fn handle_t1init_timeout(state: &mut State, ctx: &mut Context, now: SocketTime) -> bool {
     let State::CookieWait(s) = state else { unreachable!() };
-    if s.t1_init.expire(now) {
-        if s.t1_init.is_running() {
-            send_init(state, ctx);
-        } else {
-            ctx.internal_close(state, ErrorKind::TooManyRetries, "No INIT_ACK received".into());
-        }
+    if !s.t1_init.expire(now) {
+        return false;
     }
+
+    if s.t1_init.is_running() {
+        send_init(state, ctx);
+    } else {
+        ctx.internal_close(state, ErrorKind::TooManyRetries, "No INIT_ACK received".into());
+    }
+    true
 }
 
-pub(crate) fn handle_t1cookie_timeout(state: &mut State, ctx: &mut Context, now: SocketTime) {
+pub(crate) fn handle_t1cookie_timeout(
+    state: &mut State,
+    ctx: &mut Context,
+    now: SocketTime,
+) -> bool {
     let State::CookieEchoed(s) = state else { unreachable!() };
-    if s.t1_cookie.expire(now) {
-        if !s.t1_cookie.is_running() {
-            ctx.internal_close(state, ErrorKind::TooManyRetries, "No COOKIE_ACK received".into());
-        } else {
-            send_cookie_echo(state, ctx, now);
-        }
+    if !s.t1_cookie.expire(now) {
+        return false;
+    }
+
+    if !s.t1_cookie.is_running() {
+        ctx.internal_close(state, ErrorKind::TooManyRetries, "No COOKIE_ACK received".into());
+    } else {
+        send_cookie_echo(state, ctx, now);
     }
+    true
 }
 
 /// Transitions the socket to Established using the data in the Cookie.

src/socket/context.rs

@@ -97,7 +97,10 @@ impl Context {
         for packet_idx in 0..self.options.max_burst {
             if let Some(tcb) = state.tcb_mut() {
                 if packet_idx == 0 {
-                    if tcb.data_tracker.should_send_ack(now, true) {
+                    // Add SACKs if it's likely that a DATA chunk would also be added.
+                    let also_if_delayed = self.send_queue.has_data_to_send()
+                        || tcb.retransmission_queue.can_send_data();
+                    if tcb.data_tracker.should_send_ack(now, also_if_delayed) {
                         builder.add(
                             &Chunk::Sack(tcb.data_tracker.create_selective_ack(
                                 tcb.reassembly_queue.remaining_bytes() as u32,

src/socket/heartbeat.rs

@@ -71,8 +71,12 @@ pub(crate) fn handle_heartbeat_ack(ctx: &mut Context, now: SocketTime, chunk: He
     }
 }
 
-pub(crate) fn handle_heartbeat_timeouts(state: &mut State, ctx: &mut Context, now: SocketTime) {
-    if ctx.heartbeat_interval.expire(now) {
+pub(crate) fn handle_heartbeat_timeouts(
+    state: &mut State,
+    ctx: &mut Context,
+    now: SocketTime,
+) -> bool {
+    let interval_expired = if ctx.heartbeat_interval.expire(now) {
         if let Some(tcb) = state.tcb() {
             ctx.heartbeat_timeout.set_duration(ctx.options.rto_initial);
             ctx.heartbeat_timeout.start(now);
@@ -89,11 +93,20 @@ pub(crate) fn handle_heartbeat_timeouts(state: &mut State, ctx: &mut Context, no
             ));
             ctx.tx_packets_count += 1;
         }
-    }
-    if ctx.heartbeat_timeout.expire(now) {
+        true
+    } else {
+        false
+    };
+
+    let timeout_expired = if ctx.heartbeat_timeout.expire(now) {
         // Note that the timeout timer is not restarted. It will be started again when the
         // interval timer expires.
         debug_assert!(!ctx.heartbeat_timeout.is_running());
         ctx.tx_error_counter.increment();
-    }
+        true
+    } else {
+        false
+    };
+
+    interval_expired || timeout_expired
 }

src/socket/mod.rs

@@ -410,31 +410,47 @@ impl DcSctpSocket for Socket {
             return;
         }
         self.now.replace(now);
-        match &mut self.state {
-            State::Closed => {}
+        let expired_timers = match &mut self.state {
+            State::Closed => false,
             &mut State::CookieWait(ref s) => {
                 debug_assert!(s.t1_init.is_running());
-                handle_t1init_timeout(&mut self.state, &mut self.ctx, now);
+                handle_t1init_timeout(&mut self.state, &mut self.ctx, now)
             }
             State::CookieEchoed(s) => {
                 // NOTE: Only let the t1-cookie timer drive retransmissions.
                 debug_assert!(s.t1_cookie.is_running());
-                s.tcb.data_tracker.handle_timeout(now);
-                handle_t1cookie_timeout(&mut self.state, &mut self.ctx, now);
+                let ack_timer_expired = s.tcb.data_tracker.handle_timeout(now);
+                let t1_timer_expired = handle_t1cookie_timeout(&mut self.state, &mut self.ctx, now);
+                ack_timer_expired || t1_timer_expired
             }
             State::Established(tcb)
             | State::ShutdownPending(tcb)
             | State::ShutdownSent(ShutdownSentState { tcb, .. })
             | State::ShutdownReceived(tcb)
             | State::ShutdownAckSent(tcb) => {
-                tcb.data_tracker.handle_timeout(now);
-                if tcb.retransmission_queue.handle_timeout(now) {
+                let ack_timer_expired = tcb.data_tracker.handle_timeout(now);
+
+                let rtx_timer_expired = tcb.retransmission_queue.handle_timeout(now);
+                if rtx_timer_expired {
                     self.ctx.tx_error_counter.increment();
                 }
-                handle_heartbeat_timeouts(&mut self.state, &mut self.ctx, now);
-                handle_reconfig_timeout(&mut self.state, &mut self.ctx, now);
-                handle_t2_shutdown_timeout(&mut self.state, &mut self.ctx, now);
+
+                let heartbeat_timer_expired =
+                    handle_heartbeat_timeouts(&mut self.state, &mut self.ctx, now);
+                let reconfig_timer_expired =
+                    handle_reconfig_timeout(&mut self.state, &mut self.ctx, now);
+                let shutdown_timer_expired =
+                    handle_t2_shutdown_timeout(&mut self.state, &mut self.ctx, now);
+
+                ack_timer_expired
+                    || rtx_timer_expired
+                    || heartbeat_timer_expired
+                    || reconfig_timer_expired
+                    || shutdown_timer_expired
             }
+        };
+        if !expired_timers {
+            return;
         }
         if let Some(tcb) = self.state.tcb_mut() {
             if self.ctx.tx_error_counter.is_exhausted() {

src/socket/shutdown.rs

@@ -175,29 +175,36 @@ pub(crate) fn handle_shutdown_complete(
     }
 }
 
-pub(crate) fn handle_t2_shutdown_timeout(state: &mut State, ctx: &mut Context, now: SocketTime) {
+pub(crate) fn handle_t2_shutdown_timeout(
+    state: &mut State,
+    ctx: &mut Context,
+    now: SocketTime,
+) -> bool {
     let State::ShutdownSent(s) = state else {
-        return;
+        return false;
     };
-    if s.t2_shutdown.expire(now) {
-        if s.t2_shutdown.is_running() {
-            send_shutdown(state, ctx);
-            return;
-        }
+    if !s.t2_shutdown.expire(now) {
+        return false;
+    }
 
-        ctx.events.borrow_mut().add(SocketEvent::SendPacket(
-            s.tcb
-                .new_packet()
-                .add(&Chunk::Abort(AbortChunk {
-                    error_causes: vec![ErrorCause::UserInitiatedAbort(
-                        UserInitiatedAbortErrorCause { reason: "Too many retransmissions".into() },
-                    )],
-                }))
-                .build(),
-        ));
-        ctx.tx_packets_count += 1;
-        ctx.internal_close(state, ErrorKind::TooManyRetries, "Too many retransmissions".into());
+    if s.t2_shutdown.is_running() {
+        send_shutdown(state, ctx);
+        return true;
     }
+
+    ctx.events.borrow_mut().add(SocketEvent::SendPacket(
+        s.tcb
+            .new_packet()
+            .add(&Chunk::Abort(AbortChunk {
+                error_causes: vec![ErrorCause::UserInitiatedAbort(UserInitiatedAbortErrorCause {
+                    reason: "Too many retransmissions".into(),
+                })],
+            }))
+            .build(),
+    ));
+    ctx.tx_packets_count += 1;
+    ctx.internal_close(state, ErrorKind::TooManyRetries, "Too many retransmissions".into());
+    true
 }
 
 pub(crate) fn maybe_send_shutdown_on_packet_received(

src/socket/socket_tests.rs

@@ -1200,11 +1200,9 @@ mod tests {
         let packet = expect_sent_packet!(socket_a.poll_event());
         socket_z.handle_input(&packet);
         expect_on_incoming_stream_reset!(socket_z.poll_event());
-        // A <- RECONFIG <- Z
+        // A <- RECONFIG + SACK (Bundled) <- Z
         socket_a.handle_input(&expect_sent_packet!(socket_z.poll_event()));
         expect_on_streams_reset_performed!(socket_a.poll_event());
-        // A <- SACK <- Z
-        socket_a.handle_input(&expect_sent_packet!(socket_z.poll_event()));
 
         socket_a
             .send(
@@ -3479,4 +3477,49 @@ mod tests {
         let msg = socket_z.get_next_message().unwrap();
         assert_eq!(msg.payload, b"hello");
     }
+
+    #[test]
+    fn advance_time_does_not_trigger_spurious_ack() {
+        let options = default_options();
+        let mut socket_a = Socket::new("A", &options);
+        let mut socket_z = Socket::new("Z", &options);
+
+        connect_sockets(&mut socket_a, &mut socket_z);
+
+        // Send a message from A to Z. This will be the first packet, which will trigger an
+        // immediate SACK.
+        socket_a
+            .send(Message::new(StreamId(1), PpId(1), b"hello".to_vec()), &SendOptions::default())
+            .unwrap();
+
+        let packet = expect_sent_packet!(socket_a.poll_event());
+        socket_z.handle_input(&packet);
+
+        let packet = expect_sent_packet!(socket_z.poll_event());
+        let packet = SctpPacket::from_bytes(&packet, &options).unwrap();
+        assert!(packet.chunks.iter().any(|c| matches!(c, Chunk::Sack(_))));
+
+        // Send a second message from A to Z. This will be the second packet, which should trigger
+        // a delayed SACK.
+        socket_a
+            .send(Message::new(StreamId(1), PpId(1), b"hello".to_vec()), &SendOptions::default())
+            .unwrap();
+        let packet = expect_sent_packet!(socket_a.poll_event());
+
+        socket_z.handle_input(&packet);
+        expect_no_event!(socket_z.poll_event());
+
+        let next_timeout = socket_z.poll_timeout();
+        assert_eq!(next_timeout, SocketTime::zero() + options.delayed_ack_max_timeout);
+
+        // Advancing time by a small amount should NOT trigger it to send.
+        socket_z.advance_time(SocketTime::zero() + Duration::from_millis(1));
+        expect_no_event!(socket_z.poll_event());
+
+        // Advancing time by the SACK timeout should however trigger it to send.
+        socket_z.advance_time(next_timeout);
+        let packet = expect_sent_packet!(socket_z.poll_event());
+        let packet = SctpPacket::from_bytes(&packet, &options).unwrap();
+        assert!(packet.chunks.iter().any(|c| matches!(c, Chunk::Sack(_))));
+    }
 }

src/socket/stream_reset.rs

@@ -198,31 +198,37 @@ pub(crate) fn handle_reconfig(
     ctx.send_buffered_packets(state, now);
 }
 
-pub(crate) fn handle_reconfig_timeout(state: &mut State, ctx: &mut Context, now: SocketTime) {
+pub(crate) fn handle_reconfig_timeout(
+    state: &mut State,
+    ctx: &mut Context,
+    now: SocketTime,
+) -> bool {
     let tcb = state.tcb_mut().unwrap();
-    if tcb.reconfig_timer.expire(now) {
-        match tcb.current_reset_request {
-            CurrentResetRequest::None => unreachable!(),
-            CurrentResetRequest::Prepared(..) => {
-                // There is no outstanding request, but there is a prepared one. This means that
-                // the receiver has previously responded "in progress", which resulted in
-                // retrying the request (but with a new req_seq_nbr) after a while.
-            }
-            CurrentResetRequest::Inflight(..) => {
-                // There is an outstanding request, which timed out while waiting for a
-                // response.
-                ctx.tx_error_counter.increment();
-                if ctx.tx_error_counter.is_exhausted() {
-                    return;
-                }
-            }
+    if !tcb.reconfig_timer.expire(now) {
+        return false;
+    }
+
+    match tcb.current_reset_request {
+        CurrentResetRequest::None => unreachable!(),
+        CurrentResetRequest::Prepared(..) => {
+            // There is no outstanding request, but there is a prepared one. This means that
+            // the receiver has previously responded "in progress", which resulted in
+            // retrying the request (but with a new req_seq_nbr) after a while.
         }
+        CurrentResetRequest::Inflight(..) => {
+            // There is an outstanding request, which timed out while waiting for a
+            // response.
+            ctx.tx_error_counter.increment();
+        }
+    }
+    if !ctx.tx_error_counter.is_exhausted() {
         tcb.reconfig_timer.set_duration(tcb.rto.rto());
         let mut builder = tcb.new_packet();
         tcb.add_prepared_ssn_reset_request(&mut builder);
         ctx.events.borrow_mut().add(SocketEvent::SendPacket(builder.build()));
         ctx.tx_packets_count += 1;
     }
+    true
 }
 
 fn validate_req_seq_nbr(

src/tx/retransmission_queue.rs

@@ -606,6 +606,10 @@ impl RetransmissionQueue {
         to_be_sent
     }
 
+    pub fn can_send_data(&self) -> bool {
+        self.outstanding_data.has_data_to_be_retransmitted()
+    }
+
     fn chunk_max_retransmissions(&self, chunk: &DataToSend) -> u16 {
         if self.partial_reliability { chunk.max_retransmissions } else { u16::MAX }
     }