mirror of https://github.com/ospab/ostp.git
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.
This commit is contained in:
parent
7473278cc2
commit
88e0634f09
|
|
@ -295,8 +295,8 @@ impl Bridge {
|
||||||
) {
|
) {
|
||||||
match udp_msg {
|
match udp_msg {
|
||||||
Some((session_index, inbound)) => {
|
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.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 let Some(sessions) = sessions_opt.as_mut() {
|
||||||
if session_index < sessions.len() {
|
if session_index < sessions.len() {
|
||||||
let session = &mut sessions[session_index];
|
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();
|
let mut actions_queue = std::collections::VecDeque::new();
|
||||||
actions_queue.push_back(initial_action);
|
actions_queue.push_back(initial_action);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -166,6 +166,19 @@ impl ProtocolMachine {
|
||||||
self.sent_history.iter().filter(|f| f.is_retransmittable).count()
|
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 {
|
pub fn cwnd_packets(&self) -> usize {
|
||||||
self.cc.cwnd_packets() as usize
|
self.cc.cwnd_packets() as usize
|
||||||
}
|
}
|
||||||
|
|
@ -654,20 +667,34 @@ impl ProtocolMachine {
|
||||||
if !frame.is_retransmittable {
|
if !frame.is_retransmittable {
|
||||||
continue;
|
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 backoff_factor = 1u64 << (frame.retries as u64).min(6);
|
||||||
let effective_rto = Duration::from_millis(base_rto_ms.saturating_mul(backoff_factor));
|
let effective_rto = Duration::from_millis(base_rto_ms.saturating_mul(backoff_factor));
|
||||||
|
|
||||||
if now.duration_since(frame.last_sent) >= effective_rto {
|
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.last_sent = now;
|
||||||
frame.retries = frame.retries.saturating_add(1);
|
frame.retries = frame.retries.saturating_add(1);
|
||||||
|
|
||||||
if retransmit_budget > 0 {
|
|
||||||
actions.push(ProtocolAction::SendDatagram(frame.bytes.clone()));
|
actions.push(ProtocolAction::SendDatagram(frame.bytes.clone()));
|
||||||
retransmit_budget -= 1;
|
retransmit_budget -= 1;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
|
||||||
|
|
||||||
if actions.is_empty() {
|
if actions.is_empty() {
|
||||||
Ok(ProtocolAction::Noop)
|
Ok(ProtocolAction::Noop)
|
||||||
|
|
@ -1083,6 +1110,64 @@ mod tests {
|
||||||
let _ = server.on_event(OstpEvent::Tick).unwrap();
|
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.
|
/// Count how many application payloads an action tree actually delivers.
|
||||||
fn delivered_payloads(action: &ProtocolAction) -> Vec<Bytes> {
|
fn delivered_payloads(action: &ProtocolAction) -> Vec<Bytes> {
|
||||||
match action {
|
match action {
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue