From 88e0634f093abc71db90ff597a9d6ae78d2ab70c Mon Sep 17 00:00:00 2001 From: ospab Date: Thu, 30 Jul 2026 00:36:17 +0300 Subject: [PATCH] fix: two independent causes of the tunnel freezing at 0 b/s Both produce the same reported symptom - traffic stops dead, the session itself looks fine, and only a manual reconnect recovers it. 1. protocol.rs: a retry could be charged to a frame that was never sent. The retransmit loop is budget-limited per tick, but it bumped `retries` and reset `last_sent` for every due frame regardless of whether the budget actually allowed a send. The budget is smallest exactly when loss is heaviest (it is derived from cwnd, which collapses under loss), so under real packet loss frames accumulated "phantom retries" they never received - measured at 40 retries charged for 8 frames actually sent in one tick. After max_retries+2 such rounds the zombie eviction dropped them as dead. That data was never delivered and never would be: the stream stalls permanently while pings keep flowing, so nothing upstream notices anything is wrong. Retries/timers are now only charged on an actual transmit, and the loop stops scanning once the budget is spent (sent_history is in send order, so this also keeps retransmit priority oldest-first). Covered by a new test that asserts retries charged == datagrams emitted; verified it fails against the old code. 2. bridge.rs: the stall detector was reset by datagrams that never validated. `last_valid_recv` - "last VALID recv" - was assigned before decryption, so a datagram that failed to decrypt still refreshed it on its way to the error return. Anything landing on that port kept the client convinced the tunnel was healthy: frames from a session the server had already evicted, stale retransmits, or plain garbage from an off-path source that knows the ip:port. The 25s background reconnect in handle_keepalive therefore never fired. It also made the UI health indicator report a dead tunnel as fine, and gave any off-path sender a trivial way to pin a client in a dead session indefinitely. Now set only after the datagram authenticates and decrypts. --- ostp-client/src/bridge.rs | 18 +++++++- ostp-core/src/protocol.rs | 95 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 107 insertions(+), 6 deletions(-) diff --git a/ostp-client/src/bridge.rs b/ostp-client/src/bridge.rs index 74e0c19..c094924 100644 --- a/ostp-client/src/bridge.rs +++ b/ostp-client/src/bridge.rs @@ -295,8 +295,8 @@ impl Bridge { ) { match udp_msg { Some((session_index, inbound)) => { + // Raw byte counter — every datagram that reached the socket counts. self.metrics.bytes_recv.fetch_add(inbound.len() as u64, Ordering::Relaxed); - self.last_valid_recv = Instant::now(); if let Some(sessions) = sessions_opt.as_mut() { if session_index < sessions.len() { let session = &mut sessions[session_index]; @@ -309,6 +309,22 @@ impl Bridge { } }; + // Only NOW, after the datagram actually authenticated and + // decrypted, does it count as a sign of life. This used to + // be set above, before any validation — so a datagram that + // failed to decrypt still reset the stall detector on its + // way to the `return` above. Anything arriving at this port + // (frames from a session the server already evicted, stale + // retransmits, or plain garbage from an off-path source that + // knows the ip:port) kept the client convinced the tunnel + // was healthy: the 25s background reconnect in + // handle_keepalive never fired and the tunnel sat dead at + // 0 b/s until the user reconnected by hand. It also made + // `is_healthy` (see emit_metrics) lie in the UI, and handed + // any off-path sender a trivial way to pin a client in a + // dead session indefinitely. + self.last_valid_recv = Instant::now(); + let mut actions_queue = std::collections::VecDeque::new(); actions_queue.push_back(initial_action); diff --git a/ostp-core/src/protocol.rs b/ostp-core/src/protocol.rs index 620af2c..f3877eb 100644 --- a/ostp-core/src/protocol.rs +++ b/ostp-core/src/protocol.rs @@ -166,6 +166,19 @@ impl ProtocolMachine { self.sent_history.iter().filter(|f| f.is_retransmittable).count() } + /// Sum of retry counters across in-flight frames. Test-only: lets a test + /// assert the core retransmit invariant (a retry is only ever charged to a + /// frame that was actually put on the wire) without needing to advance the + /// clock through several seconds of exponential backoff. + #[cfg(test)] + fn total_retries(&self) -> usize { + self.sent_history + .iter() + .filter(|f| f.is_retransmittable) + .map(|f| f.retries as usize) + .sum() + } + pub fn cwnd_packets(&self) -> usize { self.cc.cwnd_packets() as usize } @@ -654,18 +667,32 @@ impl ProtocolMachine { if !frame.is_retransmittable { continue; } + // Out of budget for this tick — stop scanning rather than walking the + // rest of the queue. sent_history is in send order, so everything we + // skip is strictly newer than what we already handled; deferring it to + // the next tick preserves oldest-first retransmit priority. + if retransmit_budget == 0 { + break; + } let backoff_factor = 1u64 << (frame.retries as u64).min(6); let effective_rto = Duration::from_millis(base_rto_ms.saturating_mul(backoff_factor)); if now.duration_since(frame.last_sent) >= effective_rto { + // Only burn the retry counter and reset the RTO timer when the + // frame is ACTUALLY put on the wire. Doing it unconditionally + // meant that whenever the per-tick budget ran out — which is + // exactly when loss is heavy and retransmits matter most — + // frames accumulated "phantom retries" they never actually got, + // and the zombie eviction above then silently dropped them after + // `grace` such rounds. The peer never received that data and + // never would: that stream stalls forever while the session + // itself stays healthy, which is precisely the reported "tunnel + // frozen at 0 b/s but the session still up" symptom. frame.last_sent = now; frame.retries = frame.retries.saturating_add(1); - - if retransmit_budget > 0 { - actions.push(ProtocolAction::SendDatagram(frame.bytes.clone())); - retransmit_budget -= 1; - } + actions.push(ProtocolAction::SendDatagram(frame.bytes.clone())); + retransmit_budget -= 1; } } @@ -1083,6 +1110,64 @@ mod tests { let _ = server.on_event(OstpEvent::Tick).unwrap(); } + /// A retry may only be charged to a frame that was actually retransmitted. + /// + /// The retransmit loop is budget-limited per tick. It used to bump + /// `retries` and reset `last_sent` for every due frame regardless of + /// whether the budget allowed it to actually send — so under heavy loss + /// (exactly when the budget runs out) frames racked up retries they never + /// received, and the zombie eviction dropped them after `max_retries + 2` + /// such rounds. That data was never delivered and never would be: the + /// stream stalls permanently while the session itself stays up. + #[test] + fn test_retransmit_budget_charges_retries_only_for_frames_actually_sent() { + let (mut client, _server) = do_handshake(); + + // Queue far more in-flight frames than a single tick's budget allows. + const FRAMES: usize = 40; + for i in 0..FRAMES { + let payload = Bytes::from(vec![i as u8; 200]); + client.on_event(OstpEvent::Outbound(1, payload)).unwrap(); + } + assert_eq!(client.in_flight_count(), FRAMES); + assert_eq!(client.total_retries(), 0, "nothing retransmitted yet"); + + // Let every frame's RTO lapse so that on the next tick all FRAMES frames + // are due at once and the per-tick budget is guaranteed to run out. The + // effective RTO here is max(cc.rto(), config rto_ms) = 100ms at retries=0. + std::thread::sleep(Duration::from_millis(150)); + + let sent = count_datagrams(&client.on_event(OstpEvent::Tick).unwrap()); + + assert!(sent > 0, "expected some retransmits after the RTO lapsed"); + assert!( + sent < FRAMES, + "budget should have capped this tick below the {FRAMES} due frames, got {sent}" + ); + assert_eq!( + client.total_retries(), + sent, + "charged {} retries but only put {} frames on the wire — the \ + difference is phantom retries that will silently evict live data", + client.total_retries(), + sent + ); + assert_eq!( + client.in_flight_count(), + FRAMES, + "nothing was acked, so no frame may be evicted yet" + ); + } + + /// Count how many datagrams an action tree actually puts on the wire. + fn count_datagrams(action: &ProtocolAction) -> usize { + match action { + ProtocolAction::SendDatagram(_) => 1, + ProtocolAction::Multiple(list) => list.iter().map(count_datagrams).sum(), + _ => 0, + } + } + /// Count how many application payloads an action tree actually delivers. fn delivered_payloads(action: &ProtocolAction) -> Vec { match action {